Conversation
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
CodSpeed Performance ReportMerging #4697 will not alter performanceComparing Summary
|
Your org has enabled the Graphite merge queue for merging into mainAdd the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix. You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link. |
05c4a88 to
af1db2e
Compare
5d3fa08 to
dd0f1ad
Compare
dd0f1ad to
21086ad
Compare
Boshen
left a comment
There was a problem hiding this comment.
I think we should avoid our own data structures for the sake of maintainability and discoverability.
This will make it difficult for contributors to learn - remember most contributors come from JavaScript, we should lower the barrier.
As for optimization, perhaps replacing String for CompactStr is more than enough. Hot spots can also use smallvec. But reading from the heap where the vec is small won't be a performance hot spot because they'll likely live in a nearby cache.

Many rules store a
Vec<String>in their configs for things like ignore lists or JSX tags that should be checked. They are created once (infrom_configuration) and mostly used forVec::contains. There are a few problems with this:Strings are always heap-allocated. We should inline when possible, usingCompactStr.Vec::containsisO(n), and is usually used in a hot path.Vec::containsrequires an&T, meaning there are many cases when rules have an&strand need to allocate a newStringjust to pass it to contains. This is highly wasteful.Vec<String>from aValue.This PR adds
Set<T>, which seeks to make this use case more performant. It's very simple ordered set of unique items backed by aVec. Read the doc comments over the struct definition for a more in-depth rationale.Improvements
Set<T>supportsO(log(n))containment checks at the cost ofO(nlog(n))initialization. This is fine since we create it once and read from it many times.Set<T> where T: AsRef<str>>supports acontains_strmethod that is identical tocontainsbut does not force consumers to allocate new stringsSet<CompactStr>implementsTryFrom<Value>, which DRYs up rule config parsing. This trait is intentionally not implemented forSet<String>to nudge rules towards usingCompactStr.Following PRs will modify existing rules to use
Set<CompactStr>overVec<String>. Since most of these cases are within Jest and VITest rules, I don't expect improvements to appear on benchmarks (rules are skipped when non-applicable, and we have no benchmarks for Jest specs).