[PATCH] D57666: [LLD] [COFF] Avoid O(n^2) insertion into PartialSections

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: COFF/Writer.cpp:1787
+  PartialSectionKey Key = {Name, OutChars};
+  return PartialSections[Key];
 }
----------------
aganea wrote:
> `return PartialSections[{Name, OutChars}];` ?
When looking for keys not in the map, this has the side effect of creating a new (null) map entry. I think the usual .find() / != .end() dance is probably what we want instead.


================
Comment at: COFF/Writer.cpp:237
   std::unique_ptr<FileOutputBuffer> &Buffer;
-  std::set<PartialSection *, PartialLess> PartialSections;
+  std::map<PartialSectionKey, PartialSection *> PartialSections;
   std::vector<OutputSection *> OutputSections;
----------------
mstorsjo wrote:
> aganea wrote:
> > aganea wrote:
> > > On a second thought, and if we wanted to be optimal, I was wondering if we shouldn't be using a `llvm::DenseSet` here instead. `std::map` isn't known to scale particularly well with many elements, and you're at the implementation's mercy. At least the behavior will be more consistent with an internal structure.
> > > 
> > > I did use a `DenseMap` initially, but that requires more plumbing, because you need to retain the insertion order ( for later, when emitting data into the PDB). I did not see the advantage at the time, given that MSVC does not generate that many sections, but in your case, it matters. What do you think?
> > Correction: it is NOT the insertion order that matters; it is the map that needs to be enumerated in a **sorted** order on PDB emission.
> I think using `std::map` is fine - that's what was used before anyway (so this is not worse than it was before), and it is fast enough for this case. As you say, std::map/set has the upside that they are maintained sorted at all times - that's also needed in Writer when creating the OutputSections.
Hm, yes, if the number of unique keys here is large, then we might want a different data structure. For mingw, this table has practically speaking O(#symbols) entries in it. Let's leave that as a follow-up, though. Going back to std::map seems OK for now.


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

https://reviews.llvm.org/D57666