[Lldb-commits] [PATCH] D33426: Introduce new command: thread backtrace unique

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Zach made some comments via e-mail:

> Couple of things:
> 
> 1. Unless you really really want iteration over this map to be in some deterministic order, better to use unordered_map.  And if you do want it to be in some deterministic order, you should provide a comparison function, as the default one is probably not what you want.'

True. The previous solution had the same ordering as this map I proposed.

> 2. std::map<std::vector<lldb::addr_t>, std::vector<uint32_t>> is pretty horrible to read, and offers no insight into what the keys and values are.  At the very least, add some typedefs like: typedef std::vector<lldb::addr_t> StackID; typedef std::vector<tid_t> ThreadIDList; std::unordered_map<StackID, ThreadIDList> TheMap;

Agreed.

> 3. If performance is a concern here (700+ threads seems like it could exhibit slowdown when uniquing stacks), the best performance would be to use DenseSet<std::unique_ptr<StackInfo>>, where StackInfo contains both the address list and the thread is list as well as a pre-computed hash value, and then use a custom implementation of DenseMapInfo that just returns the hash value directly.  See llvm/lib/DebugInfo/CodeView/TypeSerializer.cpp for an example.

Not sure I would go that far. When you include both things in the type (StackInfo here) then you must make one up in order to search for it right? We are mainly trying to create a collection of stacks that we can use to point the the thread IDs. I agreed it doesn't need to be sorted so std::unordered_map would work just fine.

The more I think about it, the previous approach was probably fine with the std::set and with the UniqueStack class. Lets just stick with that for now, but we still need to fix the display to not show variable values and claim that many threads have the same variable values when showing ${function.name-with-args}.


https://reviews.llvm.org/D33426