[Lldb-commits] [PATCH] D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance

[Guilherme Andrade via Phabricator via lldb-commits]

guiandrade added a comment.

In D67022#1658057 <https://reviews.llvm.org/D67022#1658057>, @clayborg wrote:

> In D67022#1656874 <https://reviews.llvm.org/D67022#1656874>, @labath wrote:
>
> > Thanks for the clarification Greg.
> >
> > Functionally, this patch seems fine, but I am still wondering if we shouldn't make this more efficient. std::set is not the most memory-efficient structure, and so creating a std::set entry just to store one bit seems wasteful, particularly as there is already a container which uses the context as a key (`m_decl_ctx_to_die`). Could just shove this bit into the `m_decl_ctx_to_die` map (probably by changing it to something functionally equivalent to `map<DeclContext, pair<bool, vector<DWARFDie>>>`)? Given that the sole "raison d´être" of this map is to enable the `ParseDeclsForContext` functionality, I don't think that having that flag be stored there should be awkward. Quite the opposite, it would remove the awkward back-and-forth between the SymbolFileDWARF and the DWARFASTParsers (symbol file calls `ForEachDIEInDeclContext`, passing it a callback; then ast parser calls the callback; finally the callback immediately re-enters the parser via `GetDeclForUIDFromDWARF`) -- instead we could just have the SymbolFileDWARF do `ast_parser->PleaseParseAllDiesInThisContextIfTheyHaventBeenParsedAlready(ctx)` and have everything happen inside the parser.
> >
> > So, overall, it seems to me that this way, we could improve the code readability while reducing the time, and avoiding a bigger memory increase. In fact, since we now will not be using the list of dies after they have been parsed once, we could even free up the vector<DWARFDie> after the parsing is complete, and thereby reduce the memory footprint as well.  That would be a win-win-win scenario. :)
> >
> > WDYT?
>
>
> Great idea, but this idea gave me the idea  to use "m_decl_ctx_to_die" as is, and just remove the entry once we have parse all decls. Then we free the memory and don't need a bit. If there is no entry in the m_decl_ctx_to_die map, then ForEachDIEInDeclContext will just not iterate at all?


Thanks for the ideas, I think they greatly simplify the code. Let me know what you guys think of my implementation. I think the main issue with it is that it makes it hard to unit test it...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67022