[Lldb-commits] [PATCH] D105741: [trace] Add `thread trace dump ctf -f <filename>` command

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.


================
Comment at: lldb/include/lldb/Target/TraceHTR.h:29
+/// Metadata is initially populated in Layer 1 and merged as blocks are merged
+class HTRBlockMetadata {
+public:
----------------
This is a very expensive structure if you are going to have for Layer 1, which is just a bunch of instructions => m_num_instructions will always be 1 and m_func_calls will have 0 or 1 entries.

I'd suggest creating two classes: 

InstructionLayer (layer 0), where you could assume that m_num_instructions is 1 and instead of having the map m_func_calls, you can have an Optional<ConstString> for the function call, or you could also have a map (address -> ConstString) at the layer level that could use to look up function calls instead of storing this information in each instruction. If you follow the latter advice, the memory usage of Layer 0 would have small overhead, i.e. InstructionLayer = Instruction list + function map

BlockLayer (layer 1+), here you have blocks of 1 or more instructions. Assuming that the number of blocks will be small, you could store more metadata in each block and here statistics like number of function calls make more sense.


================
Comment at: lldb/include/lldb/Target/TraceHTR.h:41
+  size_t m_num_instructions;
+  std::unordered_map<std::string, size_t> m_func_calls;
+  friend llvm::json::Value toJSON(const HTRBlockMetadata &metadata);
----------------
clayborg wrote:
> Use ConstString here instead of std::string. std::string will make a copy of the string. ConstString objects should always be used for storing function and type names as a constant string pool will unique any strings that are placed into a ConstString. This will avoid consuming more memory for these traces and ConstString also is much more efficient for comparing strings for equality if that is needed anywhere.
See my comment above, this is too memory expensive


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105741/new/

https://reviews.llvm.org/D105741