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

Gc poll without HELPER_METHOD_FRAME #109378

Draft
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

davidwrighton
Copy link
Member

Rewrite gc polling logic as a managed helper and remove the stress helper

NOTE: I intend to build a set of assembly stubs to cover the fast path case where we just check the g_TrapReturningThreads flag, but I'd like to see how this works without that change.

…ss static data. Should result in very similar to native codegen patterns
…tics even when tls jit optimizations are disabled
Copy link
Contributor

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

src/coreclr/vm/jithelpers.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/jitinterface.h Outdated Show resolved Hide resolved
Comment on lines +454 to +456
jnz RarePath
ret
RarePath:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
jnz RarePath
ret
RarePath:
jnz JIT_PollGCRarePath
ret
JIT_PollGCRarePath:

@@ -606,5 +606,6 @@ inline BOOL ClrFlushInstructionCache(LPCVOID pCodeAddr, size_t sizeOfCode, bool
//
#define JIT_GetDynamicGCStaticBase JIT_GetDynamicGCStaticBase_SingleAppDomain
#define JIT_GetDynamicNonGCStaticBase JIT_GetDynamicNonGCStaticBase_SingleAppDomain
#define JIT_PollGC JIT_PollGC
Copy link
Member

Choose a reason for hiding this comment

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

Why are we defining the name itself?

@@ -102,6 +102,8 @@ EXTERN_C void setFPReturn(int fpSize, INT64 retVal);

#define FLOAT_REGISTER_SIZE 4 // each register in FloatArgumentRegisters is 4 bytes.

#define JIT_PollGC JIT_PollGC
Copy link
Member

Choose a reason for hiding this comment

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

Same question as above. Why same name?

Comment on lines +1188 to +1190
cbnz w9, RarePath
ret
RarePath
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cbnz w9, RarePath
ret
RarePath
cbnz w9, JIT_PollGCRarePath
ret
JIT_PollGCRarePath

Comment on lines +1161 to +1163
jnz RarePath
ret
RarePath:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
jnz RarePath
ret
RarePath:
jnz JIT_PollGCRarePath
ret
JIT_PollGCRarePath:

Comment on lines +112 to +116
#ifdef JIT_PollGC
EXTERN_C FCDECL0(void, JIT_PollGC);
#else
#define JIT_PollGC NULL
#endif
Copy link
Member

Choose a reason for hiding this comment

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

I assume this has something to do with the defining of JIT_PollGC to the same name. We should avoid this pattern in the final PR and come up with a better mechanism.

@@ -2608,49 +2608,10 @@ HRESULT EEToProfInterfaceImpl::SetEnterLeaveFunctionHooksForJit(FunctionEnter3 *

/*************************************************************/
// Slow helper to tailcall from the fast one
NOINLINE HCIMPL0(void, JIT_PollGC_Framed)
extern "C" void QCALLTYPE PollGC_Native()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this extra empty function needed? It is not referenced anywhere else. It is labeled with QCALLTYPE but does not appear in the qcallentrypoints.cpp. And this PR is already adding another empty QCALL function ThreadNative_PollGC in ``comsynchronizable.cppthat _is_ referenced inqcallentrypoints.cpp`.

#else
HCIMPL0(void, JIT_RareDisableHelper)
void JIT_RareDisableHelper()
#endif
{
// We do this here (before we set up a frame), because the following scenario
Copy link
Member

Choose a reason for hiding this comment

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

Does this comment need to be updated?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants