Page MenuHomePhabricator

[sanitizer] Move out stack trace pointer from header StackDepot
ClosedPublic

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

Details

Summary

Trace pointers accessed very rarely and don't need to
be in hot data.

Depends on D111613.

Diff Detail

Event Timeline

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

This is not immediately obvious to me that this is beneficial. When we load the first node in the chain and follow node.link's, we are chasing random pointers anyway so for every access it's a cache miss anyway, no?

compiler-rt/lib/sanitizer_common/sanitizer_stackdepot.cpp
23

restore empty line

73

This probably deserves a comment so that the next developer does not move it back to node.

This is not immediately obvious to me that this is beneficial. When we load the first node in the chain and follow node.link's, we are chasing random pointers anyway so for every access it's a cache miss anyway, no?

I guess no immediate strong benefits for now. Perf improvements on large benchmarks below the noise.
But there are some weak reasons:

  1. Less cache pressure by hot data. Amount of data we want in cache is 1/3 smaller now.
  2. System may pageout/zswap old regions of tracePtrs and traceAllocator. Maybe with help of madvice.

Even less important:

  1. It's related to next two patches: I like to think about this hash table as pure stack->id mapping. The rest we will retrive by id somehow.
  2. Easy to do now :)
  3. Flat hashtable with resize with a linear lookup when we care about cachelines. If you don't like all above I can get back with this patch after the experiment.
vitalybuka marked 2 inline comments as done.

comments

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