Page MenuHomePhabricator

[sanitizer] Remove use_count from StackDepotNode
AcceptedPublic

Authored by vitalybuka on Tue, Oct 12, 12:07 AM.

Details

Reviewers
eugenis
dvyukov
Summary

This is msan/dfsan data which does not need waste cache
of other sanitizers.

Depends on D111614.

Diff Detail

Event Timeline

vitalybuka requested review of this revision.Tue, Oct 12, 12:07 AM
vitalybuka created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptTue, Oct 12, 12:07 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript

But this does not reduce size of the node, it's still 16 bytes. So what's the point?

This comment was removed by vitalybuka.

But this does not reduce size of the node, it's still 16 bytes. So what's the point?

3-5 of https://reviews.llvm.org/D111614#3058999
in 5 I have no idea what to do with this atomic if I resize/rehash flat hash table. We will need to realloc the memory block. This called inc_use_count_unsafe so maybe under-counting is fine.

dvyukov added inline comments.Wed, Oct 13, 3:03 AM
compiler-rt/lib/sanitizer_common/sanitizer_stackdepot.cpp
63

These 2 changes combined (this and D111616) increase memory usage. Previously we had 16 byte node, after these changes we still 16 byte node, but also u32 in useCounts.

If node size is not shrinking, we can as well leave tag/size there.

It may also simplify things if we move useCounts into the trace (where we currently store size). Then we don't need a separate TwoLevelMap for useCounts.

Potentially we could get rid of tracePtrs as well, if the idx will point directly into traceAllocator (the current PersistentAllocator does not support it, but it could, or we could use something else, something like TwoLevelMap but with batch allocation where we can ask for N consecutive uptr's).

vitalybuka added inline comments.Wed, Oct 13, 10:48 AM
compiler-rt/lib/sanitizer_common/sanitizer_stackdepot.cpp
63

D111616 does not increase memory, it shares tag with size? This one increases.

These patches increment StackDepot memory usage by about 1/N where N is average frames per trace. So I don't think it's a concern.

Also it's msan-track-origins/dfsab only, for any other sanitizer it's not going to allocate anything.

It may also simplify things if we move useCounts into the trace (where we currently store size). Then we don't need a separate TwoLevelMap for useCounts.

That's not going to work.
Most memory consumed by StackDepot are traces and they are never accessed after that (tag as well). So it's go to cold storage (out of cache or even page out or even compressed, which is WIP).
But useCounts is continuously incremented by msan and it will fetch cache-lines with mostly irrelevant data, and I don't want to mix compressible constant data with mutable counter. Moving Tag like in D111616 is fine.

Actually even as-is this patch is beneficial.

So as I said this patch is NOOP for anything but msan-origins/dfsan.
I've submitted simple benchmark, _UseCount mimics msan/dfsan use:

After the patch:

autoninja -C ../out/z/ SanitizerUnitTests && ../out/z/projects/compiler-rt/lib/sanitizer_common/tests/./Sanitizer-x86_64-Test --gtest_also_run_disabled_tests --gtest_filter=*Benchmark* --gtest_repeat=1
[       OK ] SanitizerCommonBenchmarkSuite/SanitizerCommonBenchmark.DISABLED_BenchmarkInsertUniqueThreaded/3000000_10_16_UseCount (13520 ms)

Before patch:

autoninja -C ../out/z/ SanitizerUnitTests && ../out/z/projects/compiler-rt/lib/sanitizer_common/tests/./Sanitizer-x86_64-Test --gtest_also_run_disabled_tests --gtest_filter=*Benchmark* --gtest_repeat=1
[       OK ] SanitizerCommonBenchmarkSuite/SanitizerCommonBenchmark.DISABLED_BenchmarkInsertUniqueThreaded/3000000_10_16_UseCount (17402 ms)

I assume this is because nodes are now in cache lines which are not modified by other threads.

Actually even as-is this patch is beneficial.

Please disregard that message. "After" also included D111614.
So most perf benefits are form D111614 which is quite expected.

dvyukov accepted this revision.Thu, Oct 14, 5:31 AM

Also it's msan-track-origins/dfsab only, for any other sanitizer it's not going to allocate anything.

fair

This revision is now accepted and ready to land.Thu, Oct 14, 5:31 AM