This is an archive of the discontinued LLVM Phabricator instance.

[scudo] Adjust page map buffer size
ClosedPublic

Authored by Chia-hungDuan on Feb 24 2023, 12:23 PM.

Details

Summary

Given the memory group, we are unlikely to need a huge page map to
record entire region. This CL reduces the size of default page map
buffer from 2048 to 512 and increase the number of static buffers to 2.

Diff Detail

Event Timeline

Chia-hungDuan created this revision.Feb 24 2023, 12:23 PM
Chia-hungDuan requested review of this revision.Feb 24 2023, 12:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 24 2023, 12:23 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript

Update thread-safety annotation

cferris requested changes to this revision.Feb 28 2023, 2:17 PM

A couple of nits, but one question about the design.

Also, it would be good to include the data you gathered about the sizes needed to show that the 512 size was not grabbed completely out of the air.

compiler-rt/lib/scudo/standalone/release.h
62

Would it make sense to change this to a try lock? Because if you are stuck waiting for another thread to finish, it might take more time than falling back to doing the mmap. Using the try lock should be fast and avoid blocking multiple threads trying to get a block.

111–114

I know this is pulled from the previous code, but the comment should reference that this is only done for Fuchsia. We might want to say that it hasn't proven a performance benefit on other platforms.

122

This is slightly confusing. Maybe something like a '1' means that buffer index is not used. '0' means the buffer is in use.

This revision now requires changes to proceed.Feb 28 2023, 2:17 PM
Chia-hungDuan marked 3 inline comments as done.

Address review comment and slightly reorganize conditional branch

Chia-hungDuan added inline comments.Mar 3 2023, 10:57 AM
compiler-rt/lib/scudo/standalone/release.h
62

Add comment and move the getDynamicBuffer() out the lock acquiring scope.

The size is determined by collecting the buffer usage from several apps and we didn't see them over 256, most of them are smaller than 100. With partial range releasing, the required buffer size is even smaller. The following is a sample with buffer size grater than 200. It's 15 out of 312 buffer usages.

I scudo   : AllocatedUser = 28734656, Origin Buffer Size = 220, Optimized BufferSize = 177                                                                                                                         
I scudo   : AllocatedUser = 28734656, Origin Buffer Size = 220, Optimized BufferSize = 17
I scudo   : AllocatedUser = 28734656, Origin Buffer Size = 220, Optimized BufferSize = 81
I scudo   : AllocatedUser = 29947696, Origin Buffer Size = 229, Optimized BufferSize = 5 
I scudo   : AllocatedUser = 29947696, Origin Buffer Size = 229, Optimized BufferSize = 173 
I scudo   : AllocatedUser = 29947696, Origin Buffer Size = 229, Optimized BufferSize = 25
I scudo   : AllocatedUser = 29947696, Origin Buffer Size = 229, Optimized BufferSize = 181 
I scudo   : AllocatedUser = 29947696, Origin Buffer Size = 229, Optimized BufferSize = 33
I scudo   : AllocatedUser = 29947696, Origin Buffer Size = 229, Optimized BufferSize = 9 
I scudo   : AllocatedUser = 29947696, Origin Buffer Size = 229, Optimized BufferSize = 177 
I scudo   : AllocatedUser = 29947696, Origin Buffer Size = 229, Optimized BufferSize = 89
I scudo   : AllocatedUser = 29947696, Origin Buffer Size = 229, Optimized BufferSize = 177 
I scudo   : AllocatedUser = 29947696, Origin Buffer Size = 229, Optimized BufferSize = 9 
I scudo   : AllocatedUser = 29947696, Origin Buffer Size = 229, Optimized BufferSize = 41
I scudo   : AllocatedUser = 29947696, Origin Buffer Size = 229, Optimized BufferSize = 41
cferris accepted this revision.Mar 3 2023, 12:20 PM

LGTM

This revision is now accepted and ready to land.Mar 3 2023, 12:20 PM
This revision was automatically updated to reflect the committed changes.