This is an archive of the discontinued LLVM Phabricator instance.

[memprof] Keep and display symbol names in the RawMemProfReader.
ClosedPublic

Authored by snehasish on May 24 2022, 5:04 PM.

Details

Summary

Extend the Frame struct to hold the symbol name if requested
when a RawMemProfReader object is constructed. This change updates the
tests and removes the need to pass --debug to obtain the mapping from
GUID to symbol names.

Diff Detail

Event Timeline

snehasish created this revision.May 24 2022, 5:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2022, 5:04 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
snehasish requested review of this revision.May 24 2022, 5:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2022, 5:04 PM
snehasish updated this revision to Diff 431845.May 24 2022, 5:11 PM

Remove unnecessary blank lines.

tejohnson added inline comments.May 25 2022, 6:38 AM
llvm/include/llvm/ProfileData/MemProf.h
146

"should not be serialized" (missing "be")

llvm/lib/ProfileData/RawMemProfReader.cpp
419

Why not add the function name into the Frame here rather than waiting for the later call to idToFrame() and filling it from the map then?

Update comments.

snehasish marked an inline comment as done.May 25 2022, 12:09 PM

PTAL, thanks!

llvm/lib/ProfileData/RawMemProfReader.cpp
419

The idea here is to save memory by not duplicating the symbol names since we can have many unique frames (a combination of symbol + location). Also the processing does not require the symbol name today, so we only populate it in the iterator interface where a single MemProfRecord object is instantiated at a time.

tejohnson accepted this revision.May 25 2022, 1:24 PM

lgtm

llvm/lib/ProfileData/RawMemProfReader.cpp
419

Another alternative, to avoid the repeated map lookups later on, is to make SymbolName a StringRef and keep a StringSet of the unique strings for ownership.

This revision is now accepted and ready to land.May 25 2022, 1:24 PM

Thanks for the review.

llvm/lib/ProfileData/RawMemProfReader.cpp
419

While it would eliminate the map lookup, it would make it very easy to introduce lifetime issues, i.e. if the MemProfRecords outlive the RawMemProfReader the symbol name StringRef would be a dangling pointer. I'll leave it as is for now.

This revision was landed with ongoing or failed builds.May 25 2022, 2:17 PM
This revision was automatically updated to reflect the committed changes.