Performance - #22
Open
koppen wants to merge 2 commits into
Open
Conversation
Run them with `rake benchmark`:
#add with many distinct tokens n=500 0.0012s n=4000 0.0667s ratio= 54.72 max= 128.00 OK
incremental + and render on each step n=300 0.0017s n=2400 0.0852s ratio= 50.26 max= 128.00 OK
chained + with a single render at the end n=300 0.0007s n=2400 0.0280s ratio= 38.69 max= 128.00 OK
#remove with many distinct tokens n=500 0.0018s n=4000 0.1076s ratio= 59.70 max= 128.00 OK
#toggle with many distinct tokens n=500 0.0024s n=4000 0.1329s ratio= 55.74 max= 128.00 OK
By maintaining a Set alongside the ordered array of entries we can reduce the time complexity of add, toggle, include?, and replace from O(n·m) to effectively O(m) (amortized O(1) per token). - Added an @entries_set (Set) maintained alongside the ordered @entries array. include?, add, toggle's existence check, and replace's existence checks now use it for O(1) membership lookups instead of an O(n) Array#include? scan. - add and remove update both structures together; replace syncs the set when it swaps a token in place. - Added a reset(tokens) method that replaces all entries and rebuilds the set atomically — needed because Classlist::Reset was previously reaching into the raw array via entries.replace(entries), which would have silently desynced the set. Updated lib/classlist/reset.rb to call original.reset(entries) instead. Impact: add, toggle, include?, replace, and any +-chain building on them went from O(n·m) to effectively O(m) (amortized O(1) per token). Benchmarks confirm it — add at n=4000 dropped from 0.068s to 0.002s, toggle similarly. What's still O(n) per call, and why I left it: remove uses Array#delete, which has to shift elements after removing one — that's inherent to array-backed ordered storage and isn't fixable by adding a set (the set only helps skip delete calls for tokens that were never present). Fixing that would require swapping the whole backing structure (e.g. a hash + linked list), which is a much bigger change than what was asked here. Since add/toggle/chained-+ are now genuinely linear, I lowered their expected_order from 2 to 1 in test/benchmark/ — with the old threshold, a regression back to O(n²) wouldn't have tripped the guard. remove_benchmark.rb stays at order 2 with a comment explaining why.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
By maintaining a Set alongside the ordered array of entries we can reduce the time complexity of add, toggle, include?, and replace from O(n·m) to effectively O(m) (amortized O(1) per token).
Impact: add, toggle, include?, replace, and any +-chain building on them went from O(n·m) to effectively O(m) (amortized O(1) per token). Benchmarks confirm it — add at n=4000 dropped from 0.068s to 0.002s, toggle similarly.
Since add/toggle/chained-+ are now genuinely linear, I lowered their expected_order from 2 to 1 in test/benchmark/ — with the old threshold, a regression back to O(n²) wouldn't have tripped the guard. remove_benchmark.rb stays at order 2 with a comment explaining why.