-
Notifications
You must be signed in to change notification settings - Fork 171
internal/eval: performance issue with recursive definitions #803
Comments
Thanks for the repro, @verdverm. Unfortunately like #808 the reproducer is incomplete (indeed the txtar archive is invalid). If I could gently reiterate the suggestions in that issue to test out the repros as you're creating the issue - it means we can jump straight to looking at the problem, and don't have to second guess what was intended. (In this case it's probably not worth causing the program to fail in the error case - the fact that it takes a noticeable length of time to complete is sufficient). Here is my version of what I believe your repro was trying to show, which I think demonstrates the slowdown you're experiencing:
Will work with @mpvl on investigating/reducing this today. |
Analysis: this doesn't appear to be specific to the
With
Now working to reproduce this example. |
Further analysis. In the following repro, I have created a top-level
This is sufficient to reproduce the slow evaluation. What's interesting, is that moving all of the fields from
@mpvl perhaps this gives a sufficient steer as to where the problem might lie? |
Taken from cuelang/cue#803 (comment)
Further analysis. TL;DR - there is a CUE problem here; we have a couple of options for fixing it.
The observation in point 5 is, I think, the main trigger in this instance. Despite the fact that we are adding an identical declaration of What's interesting to observe is that specific conditions are required to trigger the problem, as demonstrated by points 6, 7 and 8. i.e. a slight tweak and the problem can "disappear". Therefore, we've established case 5 as the current best repro of the problem, i.e. the error case (diff with respect to the base case). For completeness I did a quick analysis over time to see whether there were any commits that stood out:
Marcel will be able to say more precisely, but I'm not convinced there is much benefit to trying to find a single commit as the root cause here. We have some pretty good ideas on the root cause. To summarise:
I also looked to see whether there is a more immediate term fix we employ here whilst the proper fix (in whatever form that takes) is being worked on. That workaround involved optimising for the fact that the "collected" value is ultimately, for now, only needed in concrete form. However, it looks like that workaround will not, unfortunately, work in this case. The reason being is that the zero value of a list type is the empty list. Hence a round trip to/from a concrete representation is lossy results in @verdverm's original repro failing because we end up trying to unify two fields values, one with a zero-length list (which should in fact be the type) with a non-zero-length list. |
Either way, it it does give an indication of what is possible. It tells me that reintroducing some of the techniques of v0.2.2 should be able to get it does to this 0.1s range Basically, the current approach filters disjunctions looking at struct properties. The theory there was that there is a limited number of syntactic element so that the number of duplicates converges to a small number quickly, stoping exponential growth, and that the check itself is quick. This turned out to be a flawed assumption: conjuncts need to be uniqued not only by syntactical element, but also by evaluator, of which there is an unbounded number. So nice try, but it doesn't work. Rather than refining the duplicate elimination model, the plan is to just brute force the comparison. This adds a constant overhead, but that beats the hell out of exponential slowdown. Also, some of the tricks now can be used to short-circuit the comparison when possible, gaining back some of the speed. Brute forcing the comparison is a bit involved: v0.3 does not compute arcs for optional fields. This would then be necessary. This chance will also help with the required field and several other features, though. |
Possible workaround: Note that
is equivalent to
as the default value of If I make that substitution, it runs well below a second. Of course there is no excuse for CUE to become so slow if the more verbose form is used, but hopefully this will help for now. |
To confirm the above hypothesis, this is a small reproducer:
Because each conjunct of Including optional fields in the equality check (as was done in v0.2, well that used subsumption, but same difference), would address this issue. Note that equality cannot be implemented accurately for bulk optional fields. But it can at least be implemented if the bulk optional field originated from the same struct. So by doing so, we achieve the dedup goal of the original v0.3 implementation, because it effectively takes the Environment factor out of the equation. Note that in this case it would be sufficient: if we rewrite A completely different approach would be to detect equivalence of Environments. This is complicated though, and is a less general mechanism. |
This issue has been migrated to cue-lang/cue#803. For more details about CUE's migration to a new home, please see cue-lang/cue#1078. |
What version of CUE are you using (
cue version
)?beta.5
What did you do?
Trying to work with Cue from the Go API. The following is extracted from a much more complex application, but captures the essence of what is happening. Some cue is loaded, some values are extracted and merged. There are notes below the
txtar
on how to change just a few lines to vary the performance and time to run.notes:
cue eval repro.cue
has no issue finishing in 0.1s// do: <...>
reduces runtime to a few seconds, but still much slowersteps: []
(an empty list) in#Build
removes the runtime issue all together.would love some help reducing this repro further
What did you expect to see?
consistent performance regardless of the line changes
What did you see instead?
CPU pinned to 100% and growing memory (via htop), program "hangs" at the
merged.Value()
line.delve has show some very strange slice sizes for internal slices (in the original), slices with lengths that would make their mem footprint in the Petabytes! Lots of time is seemingly spent in Go's garbage collector.
The text was updated successfully, but these errors were encountered: