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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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?



================
Comment at: lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp:48
   ASSERT_EQ(2u, die_list.size());
-  ASSERT_EQ(die2, die_list[0]);
-  ASSERT_EQ(die3, die_list[1]);
+  ASSERT_NE(die_list.end(), std::find(die_list.begin(), die_list.end(), die2));
+  ASSERT_NE(die_list.end(), std::find(die_list.begin(), die_list.end(), die3));
----------------
guiandrade wrote:
> I think we should not expect any particular order, right?
Probably not. A more succinct way of expressing that would be `EXPECT_THAT(die_list, testing::UnorderedElementsAre(die2, die3))`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67022