-
-
Notifications
You must be signed in to change notification settings - Fork 518
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
perf(linter): add Set #4697
perf(linter): add Set #4697
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:String
s are always heap-allocated. We should inline when possible, usingCompactStr
.Vec::contains
isO(n)
, and is usually used in a hot path.Vec::contains
requires an&T
, meaning there are many cases when rules have an&str
and need to allocate a newString
just 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_str
method that is identical tocontains
but 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).