Page MenuHomePhabricator

[scudo][standalone] Skip irrelevant regions during release
ClosedPublic

Authored by cryptoad on Aug 22 2020, 9:16 AM.

Details

Summary

With the 'new' way of releasing on 32-bit, we iterate through all the
regions in between First and Last, which covers regions that do not
belong to the class size we are working with. This is effectively wasted
cycles.

With this change, we add a SkipRegion lambda to releaseFreeMemoryToOS
that will allow the release function to know when to skip a region.
For the 64-bit primary, since we are only working with 1 region, we never
skip.

Diff Detail

Event Timeline

cryptoad created this revision.Aug 22 2020, 9:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 22 2020, 9:16 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
cryptoad requested review of this revision.Aug 22 2020, 9:16 AM
cryptoad added a subscriber: llvm-commits.

I am unclear if there is a way for the lambda to be inlined in the callee.
If anyone has a suggestion, I'll gladly take it.

I am unclear if there is a way for the lambda to be inlined in the callee.
If anyone has a suggestion, I'll gladly take it.

Nevermind this, it is actually inlined.

I am unclear if there is a way for the lambda to be inlined in the callee.
If anyone has a suggestion, I'll gladly take it.

Nevermind this, it is actually inlined.

Interesting that GCC does not inline capture by ref https://godbolt.org/z/exfP69

Interesting that GCC does not inline capture by ref https://godbolt.org/z/exfP69

Would you recommend some change to ensure inlining?

hctim accepted this revision.Aug 24 2020, 9:21 AM

LGTM.

Interesting that GCC does not inline capture by ref https://godbolt.org/z/exfP69

Would you recommend some change to ensure inlining?

There's no capture by ref here - so not a problem?

This revision is now accepted and ready to land.Aug 24 2020, 9:21 AM
vitalybuka accepted this revision.Aug 24 2020, 7:08 PM

Interesting that GCC does not inline capture by ref https://godbolt.org/z/exfP69

Would you recommend some change to ensure inlining?

LGTM. I was curious after your question and experimented a little.

cferris accepted this revision.Aug 24 2020, 10:54 PM

I verified that this appears to fix the problem we've seen, the max release was about 6ms, and that was way larger than most of the other releases.

In addition, I ran the cts test that was a slowdown before, and that ran in about the same time frame as before the change. I ran the traces too, and all numbers stayed within normal operations. I didn't run all of the smaller benchmarks since it didn't seem necessary.

I also ran all of the android tests, and everything passed.

This revision was landed with ongoing or failed builds.Aug 25 2020, 7:41 AM
This revision was automatically updated to reflect the committed changes.