Skip to content

fix(linter): OOM problems with custom plugins#17013

Merged
overlookmotel merged 6 commits intographite-base/17013from
c/12-17-draft
Dec 18, 2025
Merged

fix(linter): OOM problems with custom plugins#17013
overlookmotel merged 6 commits intographite-base/17013from
c/12-17-draft

Conversation

@camc314
Copy link
Contributor

@camc314 camc314 commented Dec 17, 2025 •
edited by overlookmotel
Loading

This PR fixes OOM (Out of Memory) problems when using JS plugins together with the import plugin for cross-module analysis.

Previously, a stopgap solution simply refused to run when >10,000 files were linted with both features enabled.

This PR implements a real fix using a deferred cloning strategy: standard allocators are used for parsing/linting (efficient, dynamic sizing), and the AST is only copied into a fixed-size allocator briefly when calling JS plugins.

This dramatically reduces memory usage from n_files × 6 GiB to n_files × dynamic + thread_count × 6 GiB.

@github-actions github-actions bot added A-linter Area - Linter A-cli Area - CLI labels Dec 17, 2025
@camc314 Graphite App
Copy link
Contributor Author

camc314 commented Dec 17, 2025 •
edited by overlookmotel
Loading

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

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.

@codspeed-hq
Copy link

codspeed-hq bot commented Dec 17, 2025 •
edited
Loading

CodSpeed Performance Report

Merging #17013 will not alter performance

Comparing c/12-17-draft (35c0e76) with c/12-17-refactor_allocator_add_docs_for_fixedsizeallocatorpool_fields_and_handle_thread_count_0 (361ca74)1

Summary

✅ 42 untouched
⏩ 3 skipped2

Footnotes

  1. No successful run was found on c/12-17-refactor_allocator_add_docs_for_fixedsizeallocatorpool_fields_and_handle_thread_count_0 (361ca74) during the generation of this report, so 7339a0a was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

  2. 3 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@camc314 camc314 force-pushed the c/12-17-draft branch 2 times, most recently from dcc634d to eec6e24 Compare December 17, 2025 15:38
@github-actions github-actions bot added the A-linter-plugins Area - Linter JS plugins label Dec 17, 2025
@camc314 camc314 changed the title draft feat(linter): Dec 17, 2025
@camc314 camc314 force-pushed the c/12-17-draft branch 3 times, most recently from c6cc9c9 to bd52e47 Compare December 17, 2025 16:50
@camc314 camc314 changed the base branch from main to graphite-base/17013 December 17, 2025 17:56
@camc314 camc314 changed the base branch from graphite-base/17013 to c/12-17-refactor_allocator_extract_capacitylimit_struct_for_pool_capacity_limiting December 17, 2025 17:56
@camc314 camc314 mentioned this pull request Dec 17, 2025
Closed
@camc314 camc314 changed the title feat(linter): fix(linter): OOM problems with custom plugins Dec 17, 2025
@github-actions github-actions bot added the C-bug Category - Bug label Dec 17, 2025
@overlookmotel
Copy link
Member

overlookmotel commented Dec 17, 2025 •
edited
Loading

IMO we should engage the "copy approach" whenever both multi-file linting and JS plugins are both in use, regardless of the file count. Because:

  • On Windows, 8 files is enough to get OOM!
  • On Mac, file count can't be trusted because multi-file analysis goes into node_modules, so it may end up linting many more files than you'd expect.

Maybe that's overkill, but I think it's better to be on the safe side, given how catastrophic it going wrong can be on Mac.

@camc314 camc314 changed the base branch from c/12-17-refactor_allocator_extract_capacitylimit_struct_for_pool_capacity_limiting to graphite-base/17013 December 17, 2025 23:07
@camc314 camc314 force-pushed the graphite-base/17013 branch from c58b1c4 to c45c03e Compare December 17, 2025 23:07
@camc314 camc314 changed the base branch from graphite-base/17013 to c/12-17-refactor_allocator_add_docs_for_fixedsizeallocatorpool_fields_and_handle_thread_count_0 December 17, 2025 23:07
This was referenced Dec 17, 2025
Merged
Merged
@camc314
Copy link
Contributor Author

camc314 commented Dec 17, 2025

IMO we should engage the "copy approach" whenever both multi-file linting and JS plugins are both in use, regardless of the file count. Because:

  • On Windows, 8 files is enough to get OOM!
  • On Mac, file count can't be trusted because multi-file analysis goes into node_modules, so it may end up linting many more files than you'd expect.

Maybe that's overkill, but I think it's better to be on the safe side, given how catastrophic it going wrong can be on Mac.

👍 agreed

@camc314 camc314 force-pushed the c/12-17-draft branch 4 times, most recently from a96406c to 36eb52a Compare December 17, 2025 23:24
@camc314 camc314 force-pushed the c/12-17-refactor_allocator_add_docs_for_fixedsizeallocatorpool_fields_and_handle_thread_count_0 branch from c45c03e to 907ddd8 Compare December 17, 2025 23:24
@overlookmotel overlookmotel force-pushed the c/12-17-refactor_allocator_add_docs_for_fixedsizeallocatorpool_fields_and_handle_thread_count_0 branch from 540bcb4 to 361ca74 Compare December 18, 2025 22:12
This was referenced Dec 18, 2025
Merged
Merged
Merged
Merged
@overlookmotel overlookmotel changed the base branch from c/12-17-refactor_allocator_add_docs_for_fixedsizeallocatorpool_fields_and_handle_thread_count_0 to graphite-base/17013 December 18, 2025 22:25
overlookmotel
overlookmotel approved these changes Dec 18, 2025
View reviewed changes
Copy link
Member

@overlookmotel overlookmotel left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

I've pushed a few commits with minor refactoring and editing comments. Will manually merge so Github retains the individual commits I pushed in case you want to check them (I did the same on the 2 other PRs in the stack).

Will follow up with more substantive changes in separate PRs.

@overlookmotel overlookmotel merged commit 776c3c7 into graphite-base/17013 Dec 18, 2025
35 of 49 checks passed
@overlookmotel overlookmotel deleted the c/12-17-draft branch December 18, 2025 22:43
@overlookmotel overlookmotel restored the c/12-17-draft branch December 18, 2025 22:47
@overlookmotel
Copy link
Member

overlookmotel commented Dec 18, 2025

Oh goddamn! I merged this PR, but I merged it into the Graphite temp branch, not main. Ergh.

Have revived the branch and opened a new PR - #17082. Will merge that now, into main this time.

This was referenced Dec 18, 2025
Merged
Merged
Merged
Merged
Merged
graphite-app bot pushed a commit that referenced this pull request Dec 18, 2025
@overlookmotel
refactor(linter): reduce repeated logic in JS plugin linting (#17086)
61f2138 
Follow-on after #17013. Pure refactor. Move logic for getting `AllocatorPool` into a function `js_allocator_pool`, to avoid lots of `#[cfg]` stuff.
graphite-app bot pushed a commit that referenced this pull request Dec 18, 2025
@overlookmotel
refactor(linter): reduce repeat code in JS plugin linting (#17087)
22124c0 
Follow-on after #17013. Refactor.

Reduce repeated code by getting a `&mut Program` out of the `ContextHost` at the start of the process, instead of in 2 places later on.

This also enables an optimization in next PR.
graphite-app bot pushed a commit that referenced this pull request Dec 18, 2025
@overlookmotel
perf(linter): avoid cloning source text when cloning AST into fixed-s…
70d853c 
…ize allocator (#17088)

Follow-on after #17013.

We already copy the source text into the new fixed-sized allocator. Prevent the source also being cloned by `program.clone_in(allocator)` - that's unnecessary as we immediately overwrite it.

This is enabled by #17087, which allows us to have a `&mut Program` at this point.
This was referenced Dec 19, 2025
Merged
Closed
Open
Closed
Closed
Closed
Closed
Merged
Merged
graphite-app bot pushed a commit that referenced this pull request Dec 19, 2025
@overlookmotel
fix(allocator): AllocatorPool do not consume all memory on Windows (#…
adb41ba 
…17094)

Modification of fixed-size allocator limits, building on #17023.

### The problem

This is an alternative design, intended to handle one flaw on Windows:

Each allocator is 4 GiB in size, so if system has 16.01 GiB of memory available, we could succeed in creating 4 x 4 GiB allocators, but that'd only leave 10 MiB of memory free. Likely then some other allocation (e.g. creating a normal `Allocator`, or even allocating a heap `String`) would fail due to OOM later on.

Note that "memory available" on Windows does not mean "how much RAM the system has". It includes the swap file, the size of which depends on how much free disk space the system has. So numbers like 16.01 GiB are not at all out of the question.

### Proposed solution

On Windows, create as many allocators as possible when creating the pool, up to `thread count + 1`. Then return the last allocator back to the system. This ensures that there's at least 4 GiB of memory free for other allocations, which should be enough.

### Redesign

In working through the various scenarios, I realized that the implementation can be simplified for both Linux/Mac and Windows.

In both cases, no more than `thread_count` fixed-size allocators can be in use at any given time - see doc comment on `FixedSizeAllocatorPool` for full explanation.

So create the pool with `thread_count` allocators (or as close as we can get on Windows). Thereafter the pool does not need to grow, and cannot.

This allows removing a bunch of synchronization code.

* On Linux/Mac, #17013 solved the too-many-allocators problem another way, so all we need is the `Mutex`.
* On Windows, we only need a `Mutex` + a `Condvar`.

In both cases, it's much simplified, which makes it much less likely for subtle race conditions like #17112 to creep in.

Removing the additional synchronization should also be a little more performant.

Note that the redesign is not the main motivator for this change - preventing OOM on Windows is.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@overlookmotel overlookmotel overlookmotel approved these changes

Assignees

@overlookmotel overlookmotel

Labels

A-cli Area - CLI A-linter Area - Linter A-linter-plugins Area - Linter JS plugins C-bug Category - Bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@camc314 @overlookmotel