fix: remove WAS_MARKED flag in favor of Set#18127
Conversation
Removes the `WAS_MARKED` logic in favor of a simple `Set` heuristic: If `mark_reactions` goes beyond a certain depth or there are many reactions on a certain signal, initialize it, otherwise keep it `null`. Balances the common case of not having many transitive dependencies with the edge case of cyclic dependencies as was seen in #16658 Fixes #18123
🦋 Changeset detectedLatest commit: 7573848 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
| count_deps += length; | ||
| // Activate the `seen` Set if we think from the unusually high number of deps that | ||
| // there might be cycles in the graph, to avoid repeated lookups for reactions | ||
| if (count_deps > 1000000 && seen === null) seen = new Set(); |
There was a problem hiding this comment.
what's the basis for this number? it seems absurdly high!
There was a problem hiding this comment.
just guts and testing against the reproduction in #16658 to see when it slows down (it does not even with this high number). But if you insist we can shave off one zero.
There was a problem hiding this comment.
It should definitely be lower — here's a repro where updates are fast on main but slow enough to drop frames on this branch:
Reducing the number closes the gap.
But it still feels very unscientific — I feel like the outcomes depend on the shape of the dependency graph, and I don't have a good intuition for when this trade-off makes sense. Clearly the set will have some overhead, and if the number is lower then it will be created more often. It's hard to tell when we would hit that threshold in real apps.
For that reason (and since it doesn't fix anything that's currently broken) I think we should hold off on merging this for the moment, even though gets rid of some code
There was a problem hiding this comment.
Fair enough - though I really hate how this logic has crept into various places, and if we do a variant of getting rid of dirty/maybe-dirty I suspect it will cause some more headscratches. So it's less about the moderate reduction in lines of code and more about the reduction in complexity.
It definitely is unscientific and just a very simple heuristic but it seems to work nicely enough. Always applying it feels worse because then everyone pays the tax / you are guaranteed to have false positives vs only sometimes.
Removes the
WAS_MARKEDlogic in favor of a simpleSetheuristic: Ifmark_reactionsgoes beyond a certain count, initialize it, otherwise keep itnull. Balances the common case of not having many transitive dependencies with the edge case of cyclic dependencies as was seen in #16658 (I rechecked that reproduction and it remains fast with this change).I ran the benchmark against this and it doesn't hit the
Seenheuristic once (i.e. even the benchmark doesn't have this extreme level of dependencies where it becomes noticeable).Fixes #18123, supersedes #18124