This is an archive of the discontinued LLVM Phabricator instance.

[scudo] Release pages of larger block more frequently
ClosedPublic

Authored by Chia-hungDuan on May 23 2023, 10:22 PM.

Details

Summary

Release pages for large block (size greater than a page) is faster than
the small blocks. Besides, larger blocks are supposed not to be used
so often like smaller blocks which means we may hold several pages used
by large block and rarely get chance to release them if there's no
explicit M_PURGE call. Therefore, relax the release-interval condition
for large block.

This also fixes the assumption that FORCE_ALL should always try page
release.

Diff Detail

Event Timeline

Chia-hungDuan created this revision.May 23 2023, 10:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2023, 10:22 PM
Chia-hungDuan requested review of this revision.May 23 2023, 10:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2023, 10:22 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript

Will measure the performance under some scenarios to see if this brings huge impact and put the number here

Use ForceAll in test instead because ForceAll always visits all the free blocks
without applying any heuristics which may skip some blocks.

Also fix the ForceAll logic in primary32

Chia-hungDuan planned changes to this revision.May 24 2023, 3:21 PM

Fix wrong logic

Adjust heuristics

If &ReleaseToOsIntervalMs == -1, still don't release large block

By some experiments,

The extra time for releasing large block more frequently is around tens of us to hundreds of us, e.g.,

scudo   : -- Average Operation Time -- -- Name (# of Calls) --
scudo   :          44426.1(ns)            releaseToOSMaybe (5284)
scudo   :          74355.5(ns)              releaseFreeMemoryToOS (2816)

scudo   : -- Average Operation Time -- -- Name (# of Calls) --
scudo   :          70457.8(ns)            releaseToOSMaybe (5219)
scudo   :          89452.3(ns)              releaseFreeMemoryToOS (3781)

scudo   : -- Average Operation Time -- -- Name (# of Calls) --
scudo   :         431321.0(ns)            releaseToOSMaybe (3398)
scudo   :         356707.4(ns)              releaseFreeMemoryToOS (3302)

The memory reduction for some apps like clash of clans, it reduces ~4% memory usage and it increases 100 MB available memory after boot. Both are measured on Android T

cferris requested changes to this revision.May 26 2023, 1:10 PM
cferris added inline comments.
compiler-rt/lib/scudo/standalone/primary32.h
137

Should there be a line between the functions?

140

Should this be >=?

781–782

There is only one way that MaySkip can be set to true and ReleaseType is not Normal. Right now, some other code is actually doing some wasted computation and other wasted work if you are doing a normal release.

For example, this MaySkip would become:

if (ReleaseType != ReleaseToOS::ForceAll)

return 0;

And all other MaySkips would become return 0.

793

It's probably worth adding a comment about why this value is chosen.

854

Should the ReleaseType check be here too?

This revision now requires changes to proceed.May 26 2023, 1:10 PM
Chia-hungDuan marked 3 inline comments as done.

Address review comment

compiler-rt/lib/scudo/standalone/primary32.h
140

My intention is to set PageSize as a boundary so I'm thinking it as "larger than". No strong opinion here, do you want it to be ">="?

854

CheckDensity has covered the check of ForceAll. But I do want to revise the check of release type a little bit in a different CL

cferris requested changes to this revision.May 26 2023, 4:42 PM

Small nit in comment.

compiler-rt/lib/scudo/standalone/primary32.h
140

I don't really care one way or another, so > is good to me too.

794

attempt

854

Sounds good to me.

This revision now requires changes to proceed.May 26 2023, 4:42 PM
Chia-hungDuan marked an inline comment as done.

Fix typo

cferris requested changes to this revision.May 30 2023, 6:42 PM

One question left.

compiler-rt/lib/scudo/standalone/primary32.h
753

This check doesn't result in a skip any more. Does one of the other checks cover the case now?

This revision now requires changes to proceed.May 30 2023, 6:42 PM
Chia-hungDuan added inline comments.May 30 2023, 9:02 PM
compiler-rt/lib/scudo/standalone/primary32.h
753

This expression was mainly for avoiding overflow when calculating the PushedBytesDelta down below, which is

const uptr PushedBytesDelta =
    BytesInFreeList - Sci->ReleaseInfo.BytesInFreeListAtLastCheckpoint;
cferris accepted this revision.May 31 2023, 10:28 AM

LGTM.

This revision is now accepted and ready to land.May 31 2023, 10:28 AM