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

Thu Jul 15 15:28:05 PDT 2021

clayborg added inline comments.

================
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);
----------------
wallace wrote:
> jj10306 wrote:
> > 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.
> > I tried using `ConstString` here, but I was running into issues because I don't believe there's a hash implementation for `ConstString` so it's unable to be the key type for an `unordered_map`. I'm considering using a `map` instead which should allow the use of `ConstString`, but at the cost of performance because lookups in these map are done quite often (every time two blocks are merged). What are your thoughts?
> See my comment above, this is too memory expensive
The constant string pool uses llvm::StringMap which ends up using "uint32_t llvm::djbHash(StringRef)". That being said, I would try and use the llvm::DenseMap instead of std::unordered_map since we already wrote the needed hashing accessors for it (search for "template <> struct DenseMapInfo<lldb_private::ConstString>" in the ConstString.h file for details).

================
Comment at: lldb/include/lldb/Target/TraceHTR.h:68
+  template <class M> friend llvm::json::Value toJSON(const TraceHTR<M> &layer);
+  template <class B> friend llvm::json::Value toJSON(const HTRBlock<B> &block);
+};
----------------
ah yes. Makes sense.

================
Comment at: lldb/include/lldb/Target/TraceHTR.h:73
+/// Maps the unique Block IDs to their respective Blocks
+template <class TMetadata> class HTRBlockDefs {
+public:
----------------
jj10306 wrote:
> wallace wrote:
> > clayborg wrote:
> > > Does this need to be templatized? Are we using this same class with multiple different TMetadata classes or are we always using it with HTRBlockMetadata?
> > +1
> > I don't think you should go with templates unless you had more TMetadata objects
> Currently, the templates are not needed because we have a single metadata type, but I added the templates to make this design extensible to different use cases that may require different metadata.
> 
> Since we currently have just a single metadata type, I'll go ahead and remove the templates and if we ever need to add a new metadata type we can discuss how to extend the design.
That sounds good to me

================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:2172
+
+  CommandObjectTraceDumpChromeTraceFormat(CommandInterpreter &interpreter)
+      : CommandObjectIterateOverThreads(
----------------
jj10306 wrote:
> clayborg wrote:
> > Instead of making a new custom command, I would say we should just add a "--export" option to the "thread trace dump" command that can take a export enumeration as its value. Possible values would be "chrome-trace-format" or "ctf". If we architect it correctly, we would create a Trace export plug-in within LLDB to allow other people to create new trace export plug-ins in the future. 
> I agree that the allowing exporting to different formats is useful, so I think the trace export plug-in is a great idea.
> 
> Discussed with @wallace offline and we decided for this diff we are going to add a `thread trace export` command that has a `--format` option and in a future diff we could add implement the plug-in logic.
Sounds good!

================
Comment at: lldb/source/Target/TraceHTR.cpp:56
+  int block_id = 0;
+  std::unordered_map<lldb::addr_t, size_t> load_address_to_block_id;
+  // This flag becomes true when cursor->Next() returns false
----------------
jj10306 wrote:
> clayborg wrote:
> > So each unique address gets added to this map? if we have many instructions that are in the same block like 0x1000, 0x1004 and 0x1008, we will have all three addresses in the map pointing to the same block ID? If we used an ordered_map, we could use lower_bound to find the lowest address that points to the block, and the block could have an address range inside of it that could be updated as we went along. It might end up saving memory if we can do it this way. 
> > 
> > So if we have:
> > ```
> > 0x1000: nop
> > 0x1004: nop
> > 0x1008: nop
> > 0x100c: call 0x2000
> > 0x2000: nop 
> > 0x2004: nop 
> > 0x2008: nop 
> > ```
> > This would end up with two blocks, one whose range is [0x1000-0x1010) (4 bytes after 0x100c), and one from [0x2000-0x200c). If we used a map the map could simply end up containing:
> > ```
> > 0x1000 -> 1 (Block [0x1000-0x1010)
> > 0x2000 -> 2 (Block [0x2000-0x200c)
> > ```
> > Using an ordered_map would take a bit more logic to always update a Block's address range on the fly and as load addresses come in, like 0x1004, 0x1008, 0x100c, we would need to locate the 0x1000 entry using "load_address_to_block_id.lower_bound(addr)" and seeing if the block's address range has been finalized, and if not, update it, and if it was, just make sure the address is in the block.
> > 
> > Where as right now the load_address_to_block_id would end up containing:
> > ```
> > 0x1000 -> 1
> > 0x1004 -> 1
> > 0x1008 -> 1
> > 0x100c -> 1
> > 0x2000 -> 2
> > 0x2004 -> 2
> > 0x2008 -> 2
> > ```
> > I worry about the size of the load_address_to_block_id data structure if it isn't minimized.
> Correct, each unique address gets an entry in this map.
> 
> For the example you gave `load_address_to_block_id` would end up containing:
> ```
> 0x1000 -> 0
> 0x1004 -> 1
> 0x1008 -> 2
> 0x100c -> 3
> 0x2000 -> 4
> 0x2004 -> 5
> 0x2008 -> 6
> ```
> The goal of this map is to assign each load address a new, unique id. The idea with assigning a new unique id is that this id could be smaller in size compared to a load address (ie 4 bytes versus 8 bytes) since the number of unique instructions in a trace is typically significantly smaller than the total number of instructions in a trace (<1% based on some testing I did) - this obviously would depend on the specific trace but the high level idea is there is potentially an opportunity to save space since it is very unlikely that any useful trace would have more than `2^32 - 1`  unique instructions. 
> 
> `size_t` is currently used as the type for the ids so this memory saving idea isn't implemented here, but wanted to get feedback on this idea.
> 
> For this diff, I'll go ahead and remove this map as a whole and just use the load addresses themselves as the unique id for a block, but I'd like to keep the idea I described above in mind for future work.
Sounds good, thanks for the explanation.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105741