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

Detect class inited status more correctly in prestub/jitinterface #104253

Merged
merged 4 commits into from
Jul 3, 2024

Conversation

davidwrighton
Copy link
Member

Fix #104247 by providing a new helper function which means what the old IsClassInited helper meant.

This should not have a semantic effect on anything, but the performance should be a bit better.

Copy link
Contributor

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

if (IsClassInited())
return TRUE;

AttemptToPreinit();
Copy link
Member

Choose a reason for hiding this comment

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

AttemptToPreinit is a bit complex method. We may end up calling it here repeatedly number of times.

Would it better to set the initialized flag during SetIsStaticDataAllocated so that we do not need to be repeating the preinit check and all calls to IsClassInited() will take care of the pre-initialization automatically?

    inline void SetIsStaticDataAllocated()
    {
        LIMITED_METHOD_CONTRACT;
        DWORD dwFlags = enum_flag_IsStaticDataAllocated;
        if (IsPreinited())
            dwFlags |= enum_flag_Initialized;
        InterlockedOr((LONG*)&m_dwFlags, (LONG)dwFlags);
    }

Copy link
Member Author

Choose a reason for hiding this comment

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

Mmm... seems like a decent idea, I'll look into that once I understand the arm64 failure seen in the tests.

Copy link
Member

Choose a reason for hiding this comment

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

The linux arm64 failure is happening on multiple PRs. Opened #104263.

…c data is allocated

This allows the IsClassInitedOrPreinited function to be implemented with a single memory read in all repeated use cases
…d correctly

- AND a very subtle race condition where we need to set the IsClassInited flag bit on dynamic statics BEFORE setting the MethodTable level one
@EgorBo
Copy link
Member

EgorBo commented Jul 2, 2024

@MihuBot

@EgorBo
Copy link
Member

EgorBo commented Jul 2, 2024

@MihuBot -nocctors

@davidwrighton davidwrighton merged commit 015b7ed into dotnet:main Jul 3, 2024
89 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MethodTable::IsClassInited returns false for <PrivateImplementationDetails> class
3 participants