Skip to content

[release/7.0] Enable caching of writeable W^X mappings #74589

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

Merged
merged 1 commit into from
Aug 25, 2022

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Aug 25, 2022

Backport of #74526 to release/7.0

/cc @janvorli

This change enables caching of the last used writeable mapping for
W^X. It was originally disabled by an ifdef, but after we've turned W^X
on by default, performance tests have shown a regression in some regex
tests. I have investigated those and found that they do excessive amount
of jitting (around 50000 methods). Enabling the caching of the last used
writeable mapping fixes the regression completely.
The caching implementation was present in the sources ever since I've
implemented the W^X stuff, but if was disabled by an ifdef. So this
change just defines the related symbol and enables the code.

The caching basically just keeps a writeable mapping after unmapping
until the next mapping request arrives, so it gives an opportunity to
reuse it in case of series of mappings of sequential range of executable
memory, which happens e.g. in the case mentioned.

Customer Impact

Performance of workloads with a lot of regular expressions usage can
regress upto 25% without this change compared to .NET 6.

Testing

CI tests, local coreclr / libraries testing, local dotnet/performance benchmarks.

Risk

Low, it just extends lifetime of a RW mapping for executable code writing / modifications until the next mapping is requested.

IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

This change enables caching of the last used writeable mapping for
W^X. It was originally disabled by an ifdef, but after we've turned W^X
on by default, performance tests have shown a regression in some regex
tests. I have investigated those and found that they do excessive amount
of jitting (around 50000 methods). Enabling the caching of the last used
writeable mapping fixes the regression completely.
The caching implementation was present in the sources ever since I've
implemented the W^X stuff, but if was disabled by an ifdef. So this
change just defines the related symbol and enables the code.

The caching basically just keeps a writeable mapping after unmapping
until the next mapping request arrives, so it gives an opportunity to
reuse it in case of series of mappings of sequential range of executable
memory, which happens e.g. in the case mentioned.
@ghost ghost added the area-VM-coreclr label Aug 25, 2022
@janvorli janvorli self-assigned this Aug 25, 2022
@janvorli janvorli added this to the 7.0.0 milestone Aug 25, 2022
@janvorli janvorli requested a review from mangod9 August 25, 2022 15:55
@carlossanlop
Copy link
Contributor

@jeffschwMSFT do you approve of this backport?
@mangod9 can we get a sign-off?

@carlossanlop
Copy link
Contributor

carlossanlop commented Aug 25, 2022

@JulieLeeMSFT can we get an approval on Jeff's behalf?

Edit: Ah, it was on @tommcdon side. Thanks.

@carlossanlop
Copy link
Contributor

CI failure is #73299
Approved and signed off.
Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit 2324d02 into release/7.0 Aug 25, 2022
@carlossanlop carlossanlop deleted the backport/pr-74526-to-release/7.0 branch August 25, 2022 18:42
@ghost ghost locked as resolved and limited conversation to collaborators Sep 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants