Skip to content

Use minimum supported PAGE_SIZE as stack probe step #45225

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

Closed
echesakov wants to merge 1 commit into from
Closed

Use minimum supported PAGE_SIZE as stack probe step #45225

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 5 additions & 2 deletions src/coreclr/src/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7419,10 +7419,13 @@ class Compiler

GenTree* eeGetPInvokeCookie(CORINFO_SIG_INFO* szMetaSig);

// Returns the page size for the target machine as reported by the EE.
// The following value is used by the JIT to determine when
// to emit a stack probing instruction sequence or a call to the stack probe helper while:
// 1. allocating a frame in a method prolog;
// 2. allocating a local heap in a method body (i.e. localloc).
target_size_t eeGetPageSize()
{
return (target_size_t)eeGetEEInfo()->osPageSize;
return 0x1000;

@jkotas jkotas Nov 25, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

@echesakov echesakov Nov 25, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually, I was planning to remove the eeGetPageSize and replace the current usages with a constant.

@jkotas jkotas Nov 25, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you also plan to delete osPageSize from JIT/EE interface? If yes, sounds good to me to keep things simple.

@echesakov echesakov Nov 25, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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?

@echesakov echesakov Nov 25, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@jkotas jkotas Nov 25, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

@jkotas jkotas Nov 25, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@sdmaclea sdmaclea Nov 25, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would agree with that for 6.0. For 5.0 servicing simpler is better.

@echesakov echesakov Nov 25, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@jkotas jkotas Nov 25, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

}

// Returns the frame size at which we will generate a loop to probe the stack.
Expand Down