Emit memory reports to debug link time memory usage #620
Conversation
This prints memory allocated by linker core data structures with a count of how many and total bytes used. This will be followed up by improvements to report virtual memory / resident memory size. Resolves #606 Signed-off-by: Shankar Easwaran <[email protected]>
quic-seaswara
force-pushed
the
memory_reports
branch
from
761a3d7 to
b751bc6
Compare
| */ | ||
| class GeneralOptions { | ||
| public: | ||
| static std::string getTypeName() { return "GeneralOptions"; } |
There was a problem hiding this comment.
Can we please return std::string_view / llvm::StringRef / const char * instead of std::string as these are more memory efficient?
| // --emit-memory-report <file> | ||
| std::string getMemoryReportFile() const { return MemoryReportFile; } | ||
|
|
||
| void setMemoryReportFile(std::string F) { MemoryReportFile = F; } |
There was a problem hiding this comment.
I think we should try to use llvm::StringRef / std::string_view wherever we can as they avoid copying strings. What do you think about it?
| void setMemoryReportFile(std::string F) { MemoryReportFile = F; } | |
| void setMemoryReportFile(llvm::StringRef F) { MemoryReportFile = F; } |
| void setPrintMemoryReport() { PrintMemoryReport = true; } | ||
|
|
||
| // --emit-memory-report <file> | ||
| std::string getMemoryReportFile() const { return MemoryReportFile; } |
There was a problem hiding this comment.
| std::string getMemoryReportFile() const { return MemoryReportFile; } | |
| llvm::StringRef getMemoryReportFile() const { return MemoryReportFile; } |
| std::string TarFile; // --reproduce output tarfile name | ||
| std::string TimingStatsFile; | ||
| std::string TimingStatsFile; // --emit-timing-stats | ||
| std::string MemoryReportFile; // --emit-memory-report |
There was a problem hiding this comment.
Can we use one option std::optional<MemoryReportFile> instead of using two options PrintMemoryReport and MemoryReportFile?
|
|
||
| // Use this arena if your object has a destructor. | ||
| // Your destructor will be invoked from freeArena(). | ||
| template <typename T, typename... U> T *make(U &&...Args) { |
There was a problem hiding this comment.
What do you think about adding the below macro for make (and slightly modifying eld::make definition) and directly using the type name supplied by the macro user to initialize the TypeName property of SpecificAlloc? I believe this will make the design significantly more maintainable and scalable.
#define MAKE(Type, ...) \
eld::make<Type>(#Type, __VA_ARGS__);
template<typename T, typename... U> T *make(llvm::StringRef TypeName, U &&... Args) {
return ArenaForType<T>::create(TypeName, std::forward<T>(Args)...);
}Now ArenaForType can directly use TypeName to initialize SpecificAlloc.
There was a problem hiding this comment.
i like this way better, though this will need every make(X) to be changed to make(T, X)
There was a problem hiding this comment.
Even currently, all make calls are of the form: make<T>(X). We still have T in the make calls :).
| bool GnuLdDriver::emitMemoryReport() const { | ||
| if (!Config.options().showMemoryReport()) | ||
| return true; | ||
| std::string File = Config.options().getMemoryReportFile(); |
There was a problem hiding this comment.
| std::string File = Config.options().getMemoryReportFile(); | |
| llvm::StringRef File = Config.options().getMemoryReportFile(); |
| */ | ||
| class BranchIsland { | ||
| public: | ||
| static std::string getTypeName() { return "BranchIsland"; } |
There was a problem hiding this comment.
Why can't we use macros to get these? Adding this to every single type feels needlessly complex and hard to scale.
There was a problem hiding this comment.
Sure. I think that is much better. Let me explore
| return "parsing_failed"; | ||
| start += strlen("parseTypeNameFromSignature<"); | ||
|
|
||
| // MSVC might add "class ", "struct ", etc. |
There was a problem hiding this comment.
Do we need this extra complexity for mscv?
| void setTimingStatsFile(std::string StatsFile) { | ||
| TimingStatsFile = StatsFile; | ||
| } | ||
| //--------------------Memory Report-------------------------------- |
There was a problem hiding this comment.
is this feature useful to anyone other than an eld developer?
There was a problem hiding this comment.
Only eld developer.
There was a problem hiding this comment.
I can make this option hidden if that is what you were hinting at.
This prints memory allocated by linker core data structures with a count of how many and total bytes used.
This will be followed up by improvements to report virtual memory / resident memory size.
Resolves #606