Skip to content

Avoid per-instance lock object in InterruptibleLazy and DelayInitArrayMap - #20088

Merged
T-Gro merged 2 commits into
dotnet:mainfrom
auduchinok:optimize-interruptiblelazy-lock
Aug 12, 2026
Merged

Avoid per-instance lock object in InterruptibleLazy and DelayInitArrayMap#20088
T-Gro merged 2 commits into
dotnet:mainfrom
auduchinok:optimize-interruptiblelazy-lock

Conversation

@auduchinok

Copy link
Copy Markdown
Member

I've been measuring memory allocations on some my changes and found out that these sync objects retail a lot of memory due to usages like in ImportILTypeDef. This is a debatable change: it makes things more dangerous (an issue could be if someone locks on the lazy itself) but at the same it's an implementation detail inside the compiler which is fairly advanced, so I hope that it can be used responsibly.

@auduchinok
auduchinok requested a review from a team as a code ownerJuly 28, 2026 10:12
@github-actionsgithub-actionsBot added the AI-Tooling-Check-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files label Jul 28, 2026
@auduchinok
auduchinokforce-pushed the optimize-interruptiblelazy-lock branch from 25074ee to bdb5b04CompareJuly 28, 2026 11:46
@majocha

Copy link
Copy Markdown
Contributor

The way System.Lazy does it is they clear the sync object field via null assignment as soon as the value is created, but it is a bit complicated.

@auduchinok

Copy link
Copy Markdown
MemberAuthor

The way System.Lazy does it is they clear the sync object field via null assignment as soon as the value is created, but it is a bit complicated.

Thanks for the suggestion! The problem is it still keeps the memory for that field, and for many thousands of types it's becoming a bit too much.

@auduchinok

Copy link
Copy Markdown
MemberAuthor

Here's an interesting discovery about how this PR overlaps with #20090:

syncObj is worth much less than its own branch measured, and that's expected. It gave −80.3 MB on FSharp.Common when addConstraintSources was importing all 107 K types (≈1 M InterruptibleLazy+syncObj pairs); with only ~35 K types imported there are proportionally fewer lock objects left, and the branch's own DelayInitValue already locks on this. Hence −14.3 MB here. The two wins overlap; they don't add.

@T-GroT-Gro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🤖 This review was generated by AI (@expert-reviewer agent). Findings may contain inaccuracies — please verify independently.

The change is mechanically correct: the private constructor makes InterruptibleLazy effectively sealed, and both DelayInitArrayMap subclasses (ILMethodDefs, ILTypeDefs) are sealed and never lock on this, so there is no internal lock collision. GetDictionary also calls GetArray() before entering its own monitor, so there is no nested self-lock. The memory win is real and worthwhile. The only caveat is the general lock-on-this concern already noted in the PR description — see the inline comment.

Comment threadsrc/Compiler/Utilities/illib.fs
@T-GroT-Gro added the AI-reviewed PR reviewed by AI review council label Aug 3, 2026
@T-Gro
T-Gro self-requested a review August 3, 2026 19:11
@auduchinok
auduchinokforce-pushed the optimize-interruptiblelazy-lock branch from bdb5b04 to a040e8aCompareAugust 10, 2026 10:08
@github-actions

github-actionsBot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

❗ Release notes required

You can open this PR in browser to add release notes: open in github.dev


✅ Found changes and release notes in following paths:

Change pathRelease notes pathDescription
`src/Compiler`docs/release-notes/.FSharp.Compiler.Service/11.0.100.md

@T-Gro

Copy link
Copy Markdown
Member

It would really great if we could annotate a type that locks itself, and then have a scanner making sure such type is never the subject to lock or Monitor.Enter.

I am more afraid of future changes regressing this, I think a one-off analysis for current snapshot is fine here.

@T-Gro
T-Gro enabled auto-merge (squash) August 12, 2026 08:46
@github-project-automationgithub-project-automationBot moved this from New to In Progress in F# Compiler and ToolingAug 12, 2026
auto-merge was automatically disabled August 12, 2026 09:02

Head branch was pushed to by a user without write access

auduchinokand others added 2 commits August 12, 2026 11:48
…yMap
Both types allocated a dedicated `syncObj = obj()` for their one-time
initialisation. These instances are internal and never locked externally,
and there are enough of them (one per lazy IL member, per ILTypeDefs /
ILMethodDefs, etc.) that the extra bare System.Object adds up to tens of MB
on a large project. Lock on `this` instead and drop the field.
Measured on a single-file FCS check against a project with ~486 references:
bare System.Object instances dropped from ~1,000,000 to ~29,000 (~-22 MB).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@auduchinok
auduchinokforce-pushed the optimize-interruptiblelazy-lock branch from 0e4e1f9 to 9416709CompareAugust 12, 2026 09:51
@T-Gro
T-Gro merged commit 29718dd into dotnet:mainAug 12, 2026
48 checks passed
@github-project-automationgithub-project-automationBot moved this from In Progress to Done in F# Compiler and ToolingAug 12, 2026
@auduchinok
auduchinok deleted the optimize-interruptiblelazy-lock branch August 12, 2026 12:33
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI-reviewedPR reviewed by AI review councilAI-Tooling-Check-Scanned-CleanTooling check: diff analyzed, no interesting infrastructure files

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants

@auduchinok@majocha@T-Gro