This is an archive of the discontinued LLVM Phabricator instance.

[scudo] Only prepare PageMap entry for partial region
ClosedPublic

Authored by Chia-hungDuan on Jan 26 2023, 12:56 PM.

Details

Summary

This reduces the size of PageMap and we are more likely to use the
static local buffer. Note that now this is only supported for single
region case, i.e. on SizeClassAllocator64. For SizeClassAllocator32,
it needs a different way to save the PageMap.

Diff Detail

Event Timeline

Chia-hungDuan created this revision.Jan 26 2023, 12:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 26 2023, 12:56 PM
Chia-hungDuan requested review of this revision.Jan 26 2023, 12:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 26 2023, 12:56 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript

Is there any performance penalty of going from a single pass to two pass approach? Is the number of blocks usually small so there isn't much of an issue?

compiler-rt/lib/scudo/standalone/primary64.h
804

Might be better worded as:

visiting BlockGroups twice.

813

of the number

821

Is there a reason you are using a comma, rather than making this two statements?

It's a bit odd to see this construct, but it might just be me.

885

I think you mean that the Next pointer can be modified. Either that or maybe use destructive to indicate the Next pointer could be changed.

888

After this loop, Prev is still pointing to the current previous block instead of Cur. Is this intended?

908

Should this be roundUpSlow?

I assume from the name that his is a slower way of doing the round up, but I might be incorrect.

910

As with the other change, making all of these Beg suffixes be Base instead make the code easier to read.

compiler-rt/lib/scudo/standalone/tests/release_test.cpp
443

logic of how

Chia-hungDuan marked 5 inline comments as done.

Address review comment

compiler-rt/lib/scudo/standalone/primary64.h
821

TBH, I simply wanted to make the induction variables updating compact (here and below). Changed them two separate lines instead.

888

Yes, we extract the current one and the Cur->Next will be the next of Prev

908

The BlockSize may not always be power-of-2 so we need to use the slower version here.

910

Done. BTW, I'm considering to also change the RegionBeg to RegionBase in another CL

Is there any performance penalty of going from a single pass to two pass approach? Is the number of blocks usually small so there isn't much of an issue?

The preliminary measurement showed that the cost of two pass was neglectable. I will bring some numbers later.

cferris requested changes to this revision.Feb 16 2023, 3:13 PM

One last question about a bit of code.

compiler-rt/lib/scudo/standalone/primary64.h
888

But BG has already been set to BG->Next. So if you hit this part of the loop twice in a row, the second time:

Prev is the same as in the previous loop
BG = BG->Next (no relation to Cur)

Did you mean BG->Next will be the next batch of Prev?

At the very least, I think a comment would help here since this is kind of confusing and look wrong.

This revision now requires changes to proceed.Feb 16 2023, 3:13 PM
  1. Add more comments on BG extraction
  2. Fix a bug in releaseToOs and add more tests
compiler-rt/lib/scudo/standalone/primary64.h
888

I guess the confusion comes from how we update Prev. Prev will be pointed to BG->Next by FreeList::extract() so the relation between Prev and BG->Next is updated.

Added more comments.

Remove debugging header

Remove more debugging stuff...

Add a performance number: The time spent in extracting/merging BatchGroup

02-21 21:04:56.996   515   515 I scudo   : -- Average Operation Time -- -- Name (# of Calls) --
02-21 21:04:56.996   515   515 I scudo   :            68.3(ns)             Extract/MergeBlock (7700)

Just a couple of comment nits.

compiler-rt/lib/scudo/standalone/primary64.h
916

a

compiler-rt/lib/scudo/standalone/tests/release_test.cpp
430

remaining

Chia-hungDuan marked 2 inline comments as done.

Address review comment

This revision was not accepted when it landed; it landed in state Needs Review.Feb 27 2023, 10:51 AM
This revision was automatically updated to reflect the committed changes.

A failed test on sanitizer-x86_64-linux-qemu. Looking into it now

MaskRay added a subscriber: MaskRay.Mar 2 2023, 1:04 PM

For relands, we usually use Reland $original_subject instead of Revert Revert $original_subject :)
Likewise, one Revert is sufficient, no need for Revert Revert Revert $original_subject (3)

I usually use Revert Dxxxx $original_subject instead of Revert Dxxxx $original_subject so that the revert commit is associated with this page. You can see the commit in "Commits" in the "Details" section.

I usually use Revert Dxxxx $original_subject instead of Revert Dxxxx $original_subject so that the revert commit is associated with this page. You can see the commit in "Commits" in the "Details" section.

Got it! Sorry I didn't notice that. Thanks