-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Remove unused members on runtime Thread #105158
Conversation
Tagging subscribers to this area: @mangod9 |
@@ -952,11 +952,6 @@ class Thread | |||
// in the object header to store it. | |||
DWORD m_ThreadId; | |||
|
|||
|
|||
// RWLock state | |||
LockEntry *m_pHead; |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 !!!! |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 <arobins@microsoft.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
I have no idea what the
LockEntry
Thread::m_embeddedEntry
andm_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