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

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to