This is an archive of the discontinued LLVM Phabricator instance.

[memprof] Deduplicate and outline frame storage in the memprof profile.
ClosedPublic

Authored by snehasish on Apr 4 2022, 4:07 PM.

Details

Summary

The current implementation of memprof information in the indexed profile
format stores the representation of each calling context fram inline.
This patch uses an interned representation where the frame contents are
stored in a separate on-disk hash table. The table is indexed via a hash
of the contents of the frame. With this patch, the compressed size of a
large memprof profile reduces by ~22%.

Diff Detail

Event Timeline

snehasish created this revision.Apr 4 2022, 4:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2022, 4:07 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
snehasish requested review of this revision.Apr 4 2022, 4:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2022, 4:07 PM

Great! Few minor comments/questions.

llvm/include/llvm/ProfileData/InstrProfWriter.h
48

From the declaration it looks like it is the reverse: holds memprof frame id to frame mappings.

llvm/include/llvm/ProfileData/MemProf.h
231

Out of curiosity, how did you choose the hashing algorithm and the value 0x9e3779b9? Is it worth documenting?

Also, the shifts on Result are unnecessary on this line since it is init to 0 just above, although if you think it is clearer to leave all the lines the same that's fine.

llvm/lib/ProfileData/InstrProfReader.cpp
972–979

RecordTableGenerator?

977

FrameTableGenerator?

llvm/lib/ProfileData/InstrProfWriter.cpp
257

Why not change the function signature to not take this parameter and then not pass it everywhere? Or do we plan to use this in the future? In the latter case, add a TODO.

409

Missing documentation for the FramePayloadOffset emitted above FrameTableOffset?

llvm/unittests/ProfileData/InstrProfTest.cpp
260

Not clear to me why a lambda is needed here since it is only invoked once directly below. Is it just for readability?

llvm/unittests/ProfileData/MemProfTest.cpp
179

Should this commented code line be here?

snehasish updated this revision to Diff 421302.Apr 7 2022, 11:33 AM

Address comments.

snehasish marked 5 inline comments as done.Apr 7 2022, 11:42 AM

Thanks for the review, ptal!

llvm/include/llvm/ProfileData/MemProf.h
231

This is inspired by boost::hash_combine. :)
Updated the code a bit here to reduce the verbosity, updated the constant to a 64-bit value (since at least one element is 64-bit) and added a comment to make things clear. There is some more info about the constant here: https://softwareengineering.stackexchange.com/a/402543

llvm/unittests/ProfileData/InstrProfTest.cpp
260

Using the Equals lambda in the prior diff allowed me to have a single place for the printing code. In this diff, I've refactored out the print code as a lambda and inlined the comparison. I don't have a strong preference for either, let me know what you think.

llvm/unittests/ProfileData/MemProfTest.cpp
179

Leftover from a prior iteration, removed.

tejohnson accepted this revision.Apr 7 2022, 12:53 PM

lgtm, couple minor things noted below.

llvm/include/llvm/ProfileData/InstrProfWriter.h
48

s/memprof id/frame id/ ?

llvm/unittests/ProfileData/InstrProfTest.cpp
260

Ah ok, that makes sense. I like the way you have it organized in the new version, it is a little clearer.

272

Nit, remove the braces around single line bodies here and below.

llvm/unittests/ProfileData/MemProfTest.cpp
179

Is the comment relevant here or should that be removed too?

This revision is now accepted and ready to land.Apr 7 2022, 12:53 PM
snehasish updated this revision to Diff 421327.Apr 7 2022, 1:22 PM

Address comments.

snehasish marked 3 inline comments as done.Apr 7 2022, 1:23 PM

Thanks for the detailed review as always. :)

This revision was landed with ongoing or failed builds.Apr 8 2022, 9:15 AM
This revision was automatically updated to reflect the committed changes.