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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo marked an inline comment as done.
mstorsjo added a comment.

In D57666#1383144 <https://reviews.llvm.org/D57666#1383144>, @aganea wrote:

> > ! In D57666#1382703 <https://reviews.llvm.org/D57666#1382703>, @mstorsjo wrote:
> >  Yeah, it's a bit inelegant to have it duplicated. I considered removing it from the value part here as well, but that would be more or less a revert of your commit I presume.
>
> No, I would need the full value to back reference from `OutputSection` (see D54802 <https://reviews.llvm.org/D54802>)


Hmm, I don't really see where PartialSection or the set/map PartialSections are used within that patch at all?

But even then, if it isn't part of the PartialSection object itself, but only part of the key in the map, can't you get it while iterating over it, like this?

  for (auto It : PartialSections) {
    const PartialSectionKey &Key = It.first;
    PartialSection *PSec = It.second;
    use(Key.Name, Key.Characteristics);



================
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;
----------------
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.


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

https://reviews.llvm.org/D57666