JitEE interface additions to support object stack allocation. #20283
Conversation
|
@echesakov @AndyAyersMS @jkotas @dotnet/jit-contrib PTAL |
src/vm/jitinterface.cpp
Outdated
| MethodTable* pMT = VMClsHnd.GetMethodTable(); | ||
| _ASSERTE(pMT); | ||
| _ASSERTE(!pMT->IsValueType()); | ||
| result = pMT->GetBaseSize(); |
There was a problem hiding this comment.
I think this should return GetNumInstanceFieldBytes(). GetBaseSize has extra padding for minimum object size. You should not need this padding for stack allocated objects.
There was a problem hiding this comment.
I changed this to GetNumInstanceFieldBytes() + OBJECT_SIZE to account for method table pointer.
src/zap/zapinfo.cpp
Outdated
| } | ||
| #endif | ||
|
|
||
| DWORD size = m_pEEJitInfo->getHeapClassSize(cls); |
There was a problem hiding this comment.
Why not just return m_pEEJitInfo->getHeapClassSize(cls); ?
src/zap/zapinfo.cpp
Outdated
| BOOL ZapInfo::classHasFinalizer(CORINFO_CLASS_HANDLE cls) | ||
| { | ||
| #ifdef FEATURE_READYTORUN_COMPILER | ||
| if (IsReadyToRunCompilation()) |
There was a problem hiding this comment.
It would be better to name this method canAllocateOnStack instead to encapsulate the policy on the EE-size.
The classes from the same version bubble can be allocated on stack just fine, even with R2R.
There was a problem hiding this comment.
It's not enough that the class being stack-allocated is from the same version bubble. I believe we need to check IsInheritanceChainLayoutFixedInCurrentVersionBubble here.
There was a problem hiding this comment.
It is fine to just return false for R2R here, for now.
There was a problem hiding this comment.
Do you think returning true if IsInheritanceChainLayoutFixedInCurrentVersionBubble returns true is incorrect?
There was a problem hiding this comment.
Do you think returning true if IsInheritanceChainLayoutFixedInCurrentVersionBubble returns true is incorrect?
Yes, it should be correct. Feel free to add it if you would like to make work properly right away.
src/inc/corinfo.h
Outdated
| CORINFO_CLASS_HANDLE cls | ||
| ) = 0; | ||
|
|
||
| virtual BOOL classHasFinalizer( |
There was a problem hiding this comment.
What is the usage pattern of these method going to be? Is getHeapClassSize going to be always called right after classHasFinalizer succeeded? If it is the case, the two methods can be combined into one to reduce chattiness.
There was a problem hiding this comment.
If classHasFinalizer returns true, getHeapClassSize won't be called.
If classHasFinalizer returns false, getHeapClassSize may or may not be called depending on the results of escape analysis.
There was a problem hiding this comment.
I can replace these methods with
BOOL canAllocateClassOnStack(CORINFO_CLASS_HANDLE cls, unsigned* classSize)
to reduce chattiness. To make it cheaper it can set classSize to 0 when it returns FALSE.
There was a problem hiding this comment.
I kept two methods but replaced classHasFinalizer with canAllocateOnStack.
|
@jkotas @AndyAyersMS I addressed all feedback, PTAL. |
| @@ -2383,6 +2383,15 @@ class ICorStaticInfo | |||
| CORINFO_CLASS_HANDLE cls | |||
There was a problem hiding this comment.
(Look for JITEEVersionIdentifier)
|
@dotnet-bot test Ubuntu arm Cross Checked Innerloop Build and Test |
Add two methods to JitEE interface: getHeapClassSize and canAllocateOnStack. Change JITEEVersionIdentifier.
erozenfeld
force-pushed
the
ObjectStackAllocationJitInterface
branch
from
cc75061 to
ff47405
Compare
|
@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test |
|
@dotnet-bot test Windows_NT x64 Checked CoreFX Tests |
|
@dotnet-bot test Windows_NT arm64 Cross Checked Innerloop Build and Test |
1 similar comment
|
@dotnet-bot test Windows_NT arm64 Cross Checked Innerloop Build and Test |
|
Windows_NT arm64 Cross Checked Innerloop Build and Test failure is a known issue. |
…#20283) Add two methods to JitEE interface: getHeapClassSize and canAllocateOnStack. Change JITEEVersionIdentifier.
Added two methods to JitEE interface: getHeapClassSize and classHasFinalizer.
They are not expected to be called when R2R compiling (they assert and return E_NOTIMPL).
Implementing them so that they are robust for versioning is a future item.
This work is tracked by #20253.