Skip to content

Conversation

@elinor-fung
Copy link
Member

@elinor-fung elinor-fung commented Jul 19, 2024

I have no idea what the LockEntry Thread::m_embeddedEntry and m_pHead pointer to it were for, but they don't appear to be used now.

Also use the SOleTlsData definition from official doc instead of our own full definition.

cc @jkotas @AaronRobinsonMSFT

@dotnet-policy-service
Copy link
Contributor

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



// RWLock state
LockEntry *m_pHead;
Copy link
Member

@jkotas jkotas Jul 20, 2024

Choose a reason for hiding this comment

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

Can we delete LockEntry completely, or at least more it to oletls.h that is its only use?

We do not need the full definition of SOleTlsData. We only need the part up until the fields that we depend on. The rest can be deleted. (A separate question is whether it is a good idea to depend on SOleTlsData that is Windows internal impl detail.)

Copy link
Member

@jkoritzinsky jkoritzinsky Jul 20, 2024

Choose a reason for hiding this comment

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

It looks like the only portion of SOleTlsData that we use is the context pointer. Would it be possible for us to use CoGetObjectContext directly instead of going through the SOleTlsData object? Then we could remove it entirely.

Copy link
Member

Choose a reason for hiding this comment

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

We would take some perf regression by replacing the shortcut with CoGetObjectContext. Also, we set the SOleTlsData field here:

_pData->pCurrentCtx = (CObjectContext*)pObjCtx; // no release !!!!
... I am not sure what to do about that.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I saw that. I wonder if we ever hit that path. My guess would be there was some bug in Windows that didn't set that sometimes so .NET Framework would do it. I really hope that path doesn't actually get hit in production...

At least since that's the only field we use, we could use the official definition of the struct from the docs for a slightly better experience.

Also, could we implement the fast path with our own thread-local variable instead of using this implementation detail?

Copy link
Member

Choose a reason for hiding this comment

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

At least since that's the only field we use, we could use the official definition of the struct from the docs for a slightly better experience.

Yeah, I think it would make sense to do it in this PR. It should be a simple change.

I agree that it would be nice to clean it up further, but I am not sure whether it is worth spending time on it. It is guaranteed to break something obscure.

Co-authored-by: Aaron Robinson <[email protected]>
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thank you!

@jkotas jkotas merged commit affd75e into dotnet:main Jul 23, 2024
@elinor-fung elinor-fung deleted the thread-unused branch July 23, 2024 14:22
@github-actions github-actions bot locked and limited conversation to collaborators Aug 23, 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.

4 participants