⚡🔎 Abridge SequenceSet#inspect output for more than 512 entries#502
+100 −5
Conversation
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
nevans changed the title nevansforce-pushed the sequence_set/abridged-inspect branch from a9f8422 to ef3a6e7CompareWhen using SequenceSet to store mailbox state for mailboxes with many millions of messages, the last thing you want to do is accidentally generate a string and output that! So this establishes a maximum number of entries to output as an inspect string, currently 512. Above that number, we output only a much smaller number of entries from the head and tail, currently 16 from each end. When the sequence set has an internal denormalized string, we use regexps to grab the first and last groups of entries from that. A note on the implementation: At first, I used a simple Regexp match anchored to `\z` to grab the tail entries from the string. But that was shockingly slow even when the string isn't extremely large. Next I benchmarked replacing `Regexp#match` with `String#rpartition` and that performed fine, using the same naive regexps. `String#rindex` was also fine. They are both slower than `Regexp#match` anchored to `\A`, but not too bad and not dramatically slower based on string size. Then I benchmarked replacing `\d+` with `\d{0,10}` or `[1-9]\d{1,9}`, and those both made the regexp fast... but `Regexp.linear_time?` doesn't like nested quantifiers. Fully expanding both the `\d+` and the `(entry){100}` keeps it fast _and_ satisfies `Regexp.linear_time?`. But that regexp is UGLY! 🙁nevansforce-pushed the sequence_set/abridged-inspect branch from ef3a6e7 to 3585984Comparenevans merged commit bac67ca into master 37 checks passed
Uh oh!
There was an error while loading. Please reload this page.
nevans commented Aug 28, 2025
| # | ||
| # This unrolls all nested quantifiers. It's much harder to read, but it's | ||
| # also the fastest out of all the versions I tested. | ||
| nz_uint32=/[1-9](?:\d(?:\d(?:\d(?:\d(?:\d(?:\d(?:\d(?:\d(?:\d)?)?)?)?)?)?)?)?)?/ |
CollaboratorAuthor
There was a problem hiding this comment.
n.b:
# max digits in nz-number(2**32 - 1).digits.count=>10# to unroll /[1-9]\d{0,9}//[1-9]#{9.times.inject(//){/(?:\d#{_1.source})?/}.source}/ == /[1-9](?:\d(?:\d(?:\d(?:\d(?:\d(?:\d(?:\d(?:\d(?:\d)?)?)?)?)?)?)?)?)?/=>truenevans changed the title SequenceSet#inspect output for more than 512 entriesnevans added the sequence-set Sign up for freeto 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.
When using SequenceSet to store mailbox state for mailboxes with many millions of messages, the last thing you want to do is accidentally generate a string and output that!
So this establishes a maximum number of entries to output as an inspect string, currently 512. Above that number, we output only a much smaller number of entries from the head and tail, currently 16 from each end.
A note on the implementation
When the sequence set has an internal denormalized string, we use regexps to grab the first and last groups of entries from that.
At first, I used a simple Regexp match anchored to
\zto grab the tail entries from the string. But that was shockingly slow even when the string isn't extremely large.Next I benchmarked replacing
Regexp#matchwithString#rpartitionand that performed fine, using the same naive regexps.String#rindexwas also fine. They are both slower thanRegexp#matchanchored to\A, but not too bad and not dramatically slower based on string size. I found myself wishing for aString#rscanorString#rsplit. 😆Then I benchmarked replacing
\d+with\d{0,10}or[1-9]\d{1,9}, and those both made the regexp fast... butRegexp.linear_time?doesn't like nested quantifiers. Fully expanding both the\d+and the(entry){100}keeps it fast and satisfiesRegexp.linear_time?.But that regexp is UGLY! 🙁