perf(linter/plugins): remove Arcs#15431
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
@camc314 Can I check it's OK to remove |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors callback types from Arc<dyn Fn...> to Box<dyn Fn...> and removes unnecessary Clone derives from Linter and ExternalLinter structs.
- Simplified callback wrapper functions by removing Arc cloning overhead
- Updated type aliases for external linter callbacks to use
Boxinstead ofArc - Removed
Clonederive from structs that no longer need to be cloneable
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/oxc_linter/src/lib.rs | Removed Clone derive from Linter struct |
| crates/oxc_linter/src/external_linter.rs | Changed callback type aliases from Arc to Box and removed Clone derive from ExternalLinter |
| apps/oxlint/src/js_plugins/external_linter.rs | Updated callback wrapper functions to use Box instead of Arc, removing Arc cloning logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Merge activity
|
don't think it's needed |
Remove 2 levels of `Arc`s from `ExternalLinter`. 1. Make wrapped callback functions `Box<dyn Fn>` instead of `Arc<dyn Fn>`. 2. Avoid wrapping `ThreadsafeFunction`s in `Arc`. The closures take ownership of them anyway. In `wrap_load_plugin`, make the inner closure borrow the `ThreadsafeFunction` from the outer closure. The first of these (and possibly the 2nd too) was prevented previously by `#[derive(Clone)]` on `Linter`. `Linter` doesn't appear to need to be cloned, so remove that derive.
CodSpeed Performance ReportMerging #15431 will not alter performanceComparing Summary
Footnotes
|
ab6bafa to
3166233
Compare
Remove 2 levels of
Arcs fromExternalLinter.Box<dyn Fn>instead ofArc<dyn Fn>.ThreadsafeFunctions inArc. The closures take ownership of them anyway. Inwrap_load_plugin, make the inner closure borrow theThreadsafeFunctionfrom the outer closure.The first of these (and possibly the 2nd too) was prevented previously by
#[derive(Clone)]onLinter.Linterdoesn't appear to need to be cloned, so remove that derive.