Skip to content
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

IL: make ILTypeDefs, ILMethodDefs thread-safe #16147

Merged
merged 3 commits into from
Oct 26, 2023

Conversation

auduchinok
Copy link
Member

@auduchinok auduchinok commented Oct 20, 2023

InlineDelayInit is used in ILTypeDefs and ILMethodDefs. It's not thread safe and creates unneeded closured for all types and methods to be possibly imported.

The thread safety problem can be seen here:

    member x.Value =
        match x.func with
        | null -> x.store
        | _ ->
            let res = LazyInitializer.EnsureInitialized(&x.store, x.func)
            x.func <- null
            res

There's a chance that x.func may be set to null on another thread before EnsureInitialized is executed, leading to exception and possibly corrupted compiler state.

It wasn't a problem before, as all this compiler logic was single-threaded. Nowadays we try to move parts of the analysis to separate threads (think about 1) skipping implementation files and then analyzing them concurrently, 2) sharing ILModuleDef between multiple background builders in an IDE, 3) moving some parts of the analysis like pattern match compilation to a later parallel stage). All of this makes it possible for this problem to occur if two threads try to import the same namespace or methods at the same time.

In addition to not being thread-safe, InlineDelayInit always allocated another closure for the dictionary factory as it used array value. It also allocated Func and kept delegates to store the factories.

The current implementation uses a monitor to fix thread safety, doesn't allocate Func delegates, and stores a single array factory lambda instead of two.

Since there're many instances created and we want to use less memory here, we can also shave another empty object and a reference field from the type if we decide that it's fine to lock on this, but that's a questionable trick. Or we could pass/use some shared object instead.

It's also possible to remove some added repetition, but I've only managed to do it at the expense of additional allocations when creating the array or dictionary, so I didn't commit these changes.

@auduchinok auduchinok requested a review from a team as a code owner October 20, 2023 10:38
@T-Gro T-Gro enabled auto-merge (squash) October 23, 2023 10:17
@T-Gro T-Gro merged commit 5ee5e58 into dotnet:main Oct 26, 2023
@auduchinok auduchinok deleted the inlineDelayInit branch October 26, 2023 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants