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%.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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? |
Thanks for the review, ptal!
llvm/include/llvm/ProfileData/MemProf.h | ||
---|---|---|
231 | This is inspired by boost::hash_combine. :) | |
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. |
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? |
From the declaration it looks like it is the reverse: holds memprof frame id to frame mappings.