Use minimum supported PAGE_SIZE as stack probe step #45225
Conversation
Dotnet-GitSync-Bot
added
the
area-CodeGen-coreclr
| target_size_t eeGetPageSize() | ||
| { | ||
| return (target_size_t)eeGetEEInfo()->osPageSize; | ||
| return 0x1000; |
There was a problem hiding this comment.
Should this rather be fixed on the EE side of the JIT/EE interface?
JIT cases should return the actual page size, AOT should return the minimum conservative size.
Otherwise, what's the point of having the osPageSize provided by the JIT/EE interface?
There was a problem hiding this comment.
Actually, I was planning to remove the eeGetPageSize and replace the current usages with a constant.
There was a problem hiding this comment.
Do you also plan to delete osPageSize from JIT/EE interface? If yes, sounds good to me to keep things simple.
There was a problem hiding this comment.
I don't see any other usages of the osPageSize field in CORINFO_EE_INFO from the JIT side other than for making stack probing decisions. If we make the JIT always use the minimum conservative size 0x1000 for both AOT and JIT scenarios then there is no purpose of having the field and function eeGetPageSize.
What approach do you think would be better?
There was a problem hiding this comment.
Do you also plan to delete osPageSize from JIT/EE interface? If yes, sounds good to me to keep things simple.
Yes, the reason I submitted the change as is to have it identical with backport change in #45226 and follow up on the cleanup at a later point.
There was a problem hiding this comment.
What approach do you think would be better?
This is tradeoff between simplicity and performance. If the actual page size is larger, you can save cycles for methods with 4k+ frames if you use the actual page size to JIT vs. the conservative minimum.
How often do we see methods with 4k+ frames?
There was a problem hiding this comment.
My gut feel is that we tend to optimize even more corner-case scenarios that this one, so it suggests that this bit of extra complexity may meet the bar.
There was a problem hiding this comment.
I would agree with that for 6.0. For 5.0 servicing simpler is better.
There was a problem hiding this comment.
How often do we see methods with 4k+ frames?
When I did #44664 I recall seeing no more than 10 methods with code differences (where the JIT replaced the inlined stack probing with a call to stack probe helper), so I would guess not many in dotnet/runtime. However, let me re-collect such statistics again to have all the number on hands.
My gut feel is that we tend to optimize even more corner-case scenarios that this one, so it suggests that this bit of extra complexity may meet the bar.
If we go with that approach, do you think it would be worth to go one step further and have multiple implementations of stack probe helpers for different page sizes (EE would specify what JIT helper to use at the process start time). For example, Arm64 we could have three implementations - for PAGE_SIZE that is 0x1000, 0x4000, 0x10000 bytes.
There was a problem hiding this comment.
multiple implementations of stack probe helpers for different page sizes
I do not think it is worth going this far. I think the most significant difference is between needing to call the helper vs. not needing to call the helper.
| target_size_t eeGetPageSize() | ||
| { | ||
| return (target_size_t)eeGetEEInfo()->osPageSize; | ||
| return 0x1000; |
There was a problem hiding this comment.
What approach do you think would be better?
delete osPageSize from JIT/EE interface (in a cleanup PR so this can be cleanly backported to 5.0)
BruceForstall
left a comment
BruceForstall
left a comment
There was a problem hiding this comment.
For the purpose of a very simple fix that can be back-ported to 5.0 and possibly earlier, this makes sense. It might be useful to a a comment such as:
Conservatively use 0x1000 (4K) as the page size. All supported platforms have pages at least
this size. For larger page sizes, this possibly leads to more stack probes than necessary
(however the stack probe code is not resilient to other page sizes currently). We must support
a scenario where crossgen runs on a larger page size than the platform where the code is
run, so we need to use a minimum conservative page size in that case.
It seems like the long-term solution is to leave the osPageSize field, but fix the VM to provide the correct value. For JIT, this can be the actual page size. For crossgen, this can be a minimum conservative page size, or the exact cross-target page size, if known. This JIT can ignore what is returned and choose 0x1000 / 4K anyway, if the JIT code incorrectly handles other page sizes.
|
Since the corresponding 5.0 change turned out not be needed I am going to close this one and come back with a change following Jan's suggestion #45225 (comment). In the meantime, I will measure the distribution of methods by their frame size to have better answer to Jan's question #45225 (comment) Thanks everyone for review! |
ghost
locked as resolved and limited conversation to collaborators
echesakov
deleted the
Use-Minimum-Supported-Page-Size-As-Stack-Probe-Step
branch
5 participants
JIT should not use the target page size for making decisions when to emit a call to a stack probe helper or inline stack probing instruction sequence. Such assumption would be wrong in AOT scenarios when crossgen compiles R2R code on a build machine with larger page size than a target machine that will be used for running this code. In theory, JIT can make such decisions in non-AOT scenarios, but the added complexity would not be justified.
@dotnet/jit-contrib