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

Stop using ThreadLocal<T>, use [ThreadStatic] #1460

Merged
merged 1 commit into from
Jan 23, 2024

Conversation

Sergio0694
Copy link
Member

We're already using [ThreadStatic] right below this, might as well just keep it consistent and use it for the stack of visited types too. This will save a little bit of binary size, as there was nothing else rooting the type otherwise (plus, one less lambda).

@Sergio0694
Copy link
Member Author

This is weird... Barely any diffs because despite dropping over 10 KB worth of stuff from ThreadLocal<T>, the new version has... Over 10 KB of seemingly random numeric methods, value typles and other stuff that just makes no sense looking at the changes:

image

@MichalStrehovsky do you have any ideas on what could be going on? 🤔

Eg. this is the trace for that value tuple... But how is any change in this PR doing any of that?

image

@MichalStrehovsky
Copy link
Member

@MichalStrehovsky do you have any ideas on what could be going on? 🤔

I don't know why is it only showing up after these changes; could be some visualization artifact or IDK, but the inclusion looks reasonable to me.

  • We ran dataflow analysis on the Marshaler<T> static constructor. It saw this assignment:
    AbiType = typeof(byte);
  • AbiType is annotated as: please make all public methods available for runtime reflection:
    #if NET
    [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)]
    #endif
    public static readonly Type AbiType;
  • So we keep byte.DivRem. If you reflection-invoke that method, it will return a boxed ValueTuple, so we need to treat ValueTuple as boxed, etc.

Note that dataflow analysis (the thing that computes dynamic use of reflection using the annotations, intrinsically recognized APIs, etc.) is a shared codebase with IL Linker and as such operates on uninstantiated method bodies. So we don't re-run it for each T but consider any T is potentially reachable when analyzing reflection use.

The only theory I have is that maybe before your change we avoided running dataflow analysis because we ended up running all Marshaler<T> static constructors at compile time. That's a bug. I filed it as dotnet/runtime#97286.

You could check whether you're running into this bug by adding <ItemGroup><IlcArg Include="--verbose" /></ItemGroup> to your project and publishing again with/without your change. The verbose log (printed by ILC to stdout) will have a list of types that were/weren't preinitialized. Try to diff that, maybe something couldn't preinitialize due to the ThreadStatic anymore.

@Sergio0694 Sergio0694 force-pushed the user/sergiopedri/drop-threadlocal branch from 0267227 to 58bfc02 Compare January 22, 2024 19:50
@Sergio0694
Copy link
Member Author

Re-run sizoscope after #1461, saves 16 KB now! 🎉

image

@Sergio0694 Sergio0694 merged commit 04088f1 into staging/AOT Jan 23, 2024
9 checks passed
@Sergio0694 Sergio0694 deleted the user/sergiopedri/drop-threadlocal branch January 23, 2024 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants