Page MenuHomePhabricator

[scudo][standalone] Secondary & general other improvements
ClosedPublic

Authored by cryptoad on Jan 27 2020, 2:03 PM.

Details

Summary

This CL changes multiple things to improve performance (notably on
Android).We introduce a class that implements a cache for the Secondary.

The changes:

  • change the Secondary "freelist" to an array. By keeping free secondary blocks linked together through their headers, we were keeping a page per block, which isn't great. Also we now touch less pages when walking the new "freelist".
  • fix an issue with the freelist getting full: if the pattern is an ever increasing size malloc then free, the freelist would fill up and entries would not be used. So now we empty the list if we get to many "full" events;
  • use the global release to os interval option for the secondary: it was too costly to release all the time, particularly for pattern that are malloc(X)/free(X)/malloc(X). Now the release will only occur after the selected interval, when going through the deallocate path;
  • allow release of the BatchClassId class: it is releasable, we just have to make sure we don't mark the batches containing batches pointers as free.
  • change the default release interval to 1s for Android to match the current Bionic allocator configuration. A patch is coming up to allow changing it through mallopt.
  • lower the smallest class that can be released to PageSize/64.

Diff Detail

Event Timeline

cryptoad created this revision.Jan 27 2020, 2:03 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 27 2020, 2:03 PM
Herald added subscribers: Restricted Project, phosek. · View Herald Transcript

Unit tests: pass. 62246 tests passed, 0 failed and 816 were skipped.

clang-tidy: fail. clang-tidy found 7 errors and 0 warnings. 0 of them are added as review comments below (why?).

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

cryptoad edited the summary of this revision. (Show Details)Jan 27 2020, 2:35 PM
eugenis accepted this revision.Jan 27 2020, 3:16 PM

LGTM

compiler-rt/lib/scudo/standalone/secondary.h
125

Would it make sense to move the IsFullEvents check here?
I.e. instead of clearing the cache after 4 failures to push a new entry, do it if the cache is X% full but we fail to find a suitable entry.

Probably would not make a difference in practice...

This revision is now accepted and ready to land.Jan 27 2020, 3:16 PM
hctim added inline comments.Jan 27 2020, 3:43 PM
compiler-rt/lib/scudo/standalone/flags.inc
48

Given that this might get very complicated if you wanted to introduce different defaults for different platforms - it may be worth adding a TODO to initialise Scudo as part of libc_init_malloc rather than rely on lazy-initialisation.

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

Might be worth considering

compiler-rt/lib/scudo/standalone/secondary.h
59

UNUSED uptr Size

146

Can you reuse code via. empty() { releaseOlderThan(UINT64_MAX) }?

193

Missing EntriesCount -= N?

294–296

Nit: Just canCache(RoundedSize)

387

Nit: just canCache(CommitSize)

pcc added inline comments.Jan 27 2020, 3:53 PM
compiler-rt/lib/scudo/standalone/flags.inc
48

I would prefer if we moved towards making these things static (e.g. part of AndroidConfig et al) rather than configurable via environment variables.

cryptoad marked 4 inline comments as done.Jan 27 2020, 3:58 PM
cryptoad added inline comments.
compiler-rt/lib/scudo/standalone/flags.inc
48

Ack. Technically a platform could also use the SCUDO_DEFAULT_OPTIONS define at compile time.

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

In the works with https://reviews.llvm.org/D72882. I'll come back to that after this lands as it's higher priority.

compiler-rt/lib/scudo/standalone/secondary.h
146

I looked into this, as the constructs are similar. One of the issues is that one works with Map* members, the other with Block* members.
In the end I decided against as it became ugly, but I can revisit this at a later point hopefully.

193

The entries are still usable, we released the memory but they can be reused at some point (unlike unmapping). It will incur faulting, which is slower, but has a positive impact on the RSS.

hctim accepted this revision.Jan 27 2020, 4:07 PM

Acks, LGTM.

cryptoad updated this revision to Diff 240778.Jan 27 2020, 11:25 PM

Marking a parameter unused.

Unit tests: pass. 62248 tests passed, 0 failed and 816 were skipped.

clang-tidy: fail. clang-tidy found 7 errors and 0 warnings. 0 of them are added as review comments below (why?).

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

cryptoad updated this revision to Diff 240866.Jan 28 2020, 6:53 AM

Passing ReleaseToOsInterval from init to initLinkerInitialized in
the Secondary.

cryptoad marked an inline comment as done.Jan 28 2020, 7:05 AM
cryptoad added inline comments.
compiler-rt/lib/scudo/standalone/flags.inc
48

I think making it part of the configuration is a good idea once we get a way to change it at runtime which is coming up with the mallopt change.

Unit tests: pass. 62248 tests passed, 0 failed and 816 were skipped.

clang-tidy: fail. clang-tidy found 7 errors and 0 warnings. 0 of them are added as review comments below (why?).

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

This revision was automatically updated to reflect the committed changes.