-
-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix skewedIndex becoming outrageously big and document tradeoffs of our decisions #4483
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📊 Tachometer Benchmark ResultsSummaryduration
usedJSHeapSize
Resultscreate10k
duration
usedJSHeapSize
filter-list
duration
usedJSHeapSize
hydrate1k
duration
usedJSHeapSize
many-updates
duration
usedJSHeapSize
replace1k
duration
usedJSHeapSize
run-warmup-0
run-warmup-1
run-warmup-2
run-warmup-3
run-warmup-4
run-final
text-update
duration
usedJSHeapSize
todo
duration
usedJSHeapSize
update10th1k
duration
usedJSHeapSize
|
JoviDeCroock
force-pushed
the
fixes-to-diffing
branch
from
August 27, 2024 16:17
363bfe5
to
a6c3cff
Compare
Size Change: -118 B (-0.19%) Total Size: 62 kB
ℹ️ View Unchanged
|
JoviDeCroock
changed the title
Fix skewedIndex becoming outrageously sized and document tradeoffs of our decisions
Fix skewedIndex becoming outrageously big and document tradeoffs of our decisions
Aug 27, 2024
marvinhagemeister
approved these changes
Aug 28, 2024
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #4482
When we move elements around i.e. [0, 1, 2] --> [1, 0, 2]
This becomes an optimization question where currently we see a 1 element offset as an insertion or deletion i.e. we optimize for [0, 1, 2] --> [9, 0, 1, 2]
while a more than 1 offset we see as a swap.
We could probably build heuristics for having an optimized course of action here as well, but
might go at the cost of some bytes.
If we wanted to optimize for i.e. only swaps we'd just do the last two code-branches and have
only the first item be a re-scouting and all the others fall in their skewed counter-part.
We could also further optimize for swaps.
There are possible tweaks in how we mark a node for insertion, however I don't think it's in scope for this PR and prefer addressing the performance downgrade. Heck we could even remove the optimization for inserts/deletions and save 20-30 more bytes by only relying on
>|<
.A more elaborate example
Basically for the case of:When we follow the new-old algorithm we have now, we know that
This is no bug because the placeChild algorithm but feels like we could do a lot better in this algorithm by seeing that the length has stayed the same and comparing old to new instead 😅
Ideally we reset the skew again after diffing c