Performance - #22
Open
koppen wants to merge 2 commits into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves Classlist performance by maintaining a Set alongside the ordered @entries array for O(1) membership checks, and adds a lightweight benchmark harness + rake task to guard against algorithmic regressions in core operations.
Changes:
- Introduce
@entries_setto accelerateinclude?,add,toggle, and parts ofremove/replace, and add areset(tokens)method to rebuild both structures in sync. - Update
Classlist::Resetto calloriginal.reset(entries)instead of mutatingoriginal.entriesdirectly (avoids desynchronizing the set). - Add benchmark regression guards under
test/benchmark/and arake benchmarktask to run them.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
lib/classlist.rb |
Adds Set-backed membership tracking, sync logic in mutators, and a new reset(tokens) API. |
lib/classlist/reset.rb |
Switches reset operation to use Classlist#reset to keep internal structures consistent. |
Rakefile |
Adds a benchmark rake task to run scaling-regression checks. |
test/benchmark/benchmark_helper.rb |
Introduces a small harness (Bench.scale_check) for algorithmic scaling checks. |
test/benchmark/add_benchmark.rb |
Adds scaling guard for #add with many distinct tokens. |
test/benchmark/toggle_benchmark.rb |
Adds scaling guard for #toggle with many distinct tokens. |
test/benchmark/remove_benchmark.rb |
Adds scaling guard for #remove and documents expected O(n²) behavior for bulk removal. |
test/benchmark/merge_operations_benchmark.rb |
Adds scaling guard for chained + operations with a single render. |
test/benchmark/incremental_add_and_render_benchmark.rb |
Adds scaling guard for incremental + + render pattern (historical regression). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| require_relative "benchmark_helper" | ||
|
|
||
| # Regression guard for https://github.com/substancelab/classlist/issues - |
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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
lib/classlist.rb:106
resetreturns the internal@entriesarray. Since the class now maintains an@entries_setcache, returning a mutable reference makes it easy for callers to mutate the array and desync the set. Returning a defensive copy avoids this footgun without affecting internal behavior.
def reset(tokens)
@entries = build_entries(tokens)
@entries_set = @entries.to_set
@entries
end
Comment on lines
62
to
64
| def include?(token) | ||
| entries.include?(token) | ||
| @entries_set.include?(token) | ||
| end |
Comment on lines
+32
to
+36
| size_ratio = large.to_f / small | ||
| max_allowed_ratio = (size_ratio**expected_order) * tolerance | ||
| actual_ratio = small_time.zero? ? 0 : large_time / small_time | ||
|
|
||
| ok = actual_ratio <= max_allowed_ratio |
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
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.