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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
