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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jul 23, 2024
Merged

Remove unused members on runtime Thread #105158

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1eb07c0
Remove unused members on runtime Thread
elinor-fung Jul 19, 2024
173981e
Use SOleTlsData definition from public doc
elinor-fung Jul 22, 2024
c440103
Remove forward declaration
elinor-fung Jul 22, 2024
8430348
Update src/coreclr/vm/oletls.h
elinor-fung Jul 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions src/coreclr/vm/threads.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1330,14 +1330,6 @@ Thread::Thread()

m_dwForbidSuspendThread = 0;

// Initialize lock state
m_pHead = &m_embeddedEntry;
m_embeddedEntry.pNext = m_pHead;
m_embeddedEntry.pPrev = m_pHead;
m_embeddedEntry.dwLLockID = 0;
m_embeddedEntry.dwULockID = 0;
m_embeddedEntry.wReaderLevel = 0;

m_pBlockingLock = NULL;

m_pRuntimeThreadLocals = nullptr;
Expand Down
5 changes: 0 additions & 5 deletions src/coreclr/vm/threads.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -952,11 +952,6 @@ class Thread
// in the object header to store it.
DWORD m_ThreadId;


// RWLock state
LockEntry *m_pHead;
Copy link
Member

@jkotas jkotas Jul 20, 2024 •
edited
Loading

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 •
edited
Loading

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:

runtime/src/coreclr/vm/olecontexthelpers.cpp

Line 54 in 043ae8c

_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.

LockEntry m_embeddedEntry;

#ifndef DACCESS_COMPILE
Frame* NotifyFrameChainOfExceptionUnwind(Frame* pStartFrame, LPVOID pvLimitSP);
#endif // DACCESS_COMPILE
Expand Down
Loading