[PATCH] D121006: Speedup dsymutil when working with big project.

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a subscriber: lebedev.ri.
dexonsmith added a comment.

I just came in to comment on the API change — seems like ArrayRef would be better! — but a spotted a couple of other things.

(I don't have much context on the logic here in dsymutil, so these are just suggestions; probably @JDevlieghere or @lebedev.ri will need to LGTM; but thought I'd share what I noticed.)



================
Comment at: llvm/tools/dsymutil/MachODebugMapParser.cpp:83
   uint64_t getMainBinarySymbolAddress(StringRef Name);
-  std::vector<StringRef> getMainBinarySymbolNames(uint64_t Value);
+  SmallVector<StringRef> &getMainBinarySymbolNames(uint64_t Value);
   void loadMainBinarySymbols(const MachOObjectFile &MainBinary);
----------------
An API like this should probably return an `ArrayRef<StringRef>` (immutable; doesn't leak the internal storage type) rather than `SmallVector<StringRef>`. Although my other inline comments point in a different direction...


================
Comment at: llvm/tools/dsymutil/MachODebugMapParser.cpp:594-595
+    if (Extern) {
       MainBinarySymbolAddresses[Name] = Addr;
-    else
-      MainBinarySymbolAddresses.try_emplace(Name, Addr);
+      getMainBinarySymbolNames(Addr).push_back(Name);
+    } else {
----------------
It's not obvious to me that `MachODebugMapParser::getMainBinarySymbolNames()` is going to return the same things as before. If this replaces a different `Addr`, then there will be two different `Addrs` that have this symbol.

I'm not sure if the precise, existing behaviour is important, but if it is, I think you'll need a second pass through. I think something like this would work (outside/after the `for` loop that's building up these data structures):
```
lang=c++
for (auto &AliasList : MainBinarySymbolAddress2Name) {
  AliasList.second.erase(
      llvm::remove_if(AliasList.second,
          [&](StringMapEntry<uint64_t> *Entry) {
            return Entry->second != AliasList.first;
          }),
      AliasList.second.end());
}
```


================
Comment at: llvm/tools/dsymutil/MachODebugMapParser.cpp:58
+  /// `getMainBinarySymbolNames`;
+  std::unordered_map<uint64_t, std::vector<StringRef>>
+      MainBinaryAddresses2NamesMap;
----------------
C-_-fan wrote:
> lebedev.ri wrote:
> > `std::unordered_map` has bad performance characteristics.
> > Does it help if this is instead a SmallDenseMap of SmallVector's? 
> Thank you for your review and comment， I'll update and test later.
You could refine this a bit I think:

Firstly, storing `SmallVector<StringRef>` on the heap is a bit questionable, since `SmallVector` includes non-trivial small storage. Probably you want `SmallVector<StringRef, 1>` to customize the small storage for the common case (assuming 1 symbol per address is the common case?).

Secondly, `StringRef` is the size of two pointers, but you could use a `StringMapEntry<uint64_t> *` into `MainBinarySymbolAddresses` to halve the storage. You'd want to update:
```
lang=c++
    if (Extern)
      MainBinarySymbolAddresses[Name] = Addr;
    else
      MainBinarySymbolAddresses.try_emplace(Name, Addr);
```
to:
```
lang=c++
    auto I = MainBinarySymbolAddresses.try_emplace(Name, Addr);
    // Replace the na
    if (Extern && !I.second)
      I.first->second = Addr;
    if (Extern || I.second)
      MainBinaryAddressses2Names[Addr].push_back(&*I.first);
```

Writing that suggested code, I have reproduced what I think is a logic bug, where the API now returns something different than before your patch. See my other inline comment.

Thirdly, I think you could probably do something complicated with a flat `SmallVector` and augment the data in `MainBinarySymbolAddresses` with two `uint32_t`s to indicate the range within it. More complicated, but probably less memory overall, faster for accessing, and maybe faster for building (depending on various characteristics). Here is the basic model:
- Change MainBinarySymbolAddresses to store a struct that has an `uint32_t` index; maybe called `SymbolAddressData`; index starts out as `-1U`.
- During the loop, build up `SmallVector<StringMapEntry<SymbolAddressData> *>` as you go, only pushing back on insertions.
- Use a stable sort to make the vector ordered by pointed-address, so aliases are adjacent to each other.
- Do a pass through the vector. For each address:
    - If there's only one symbol with that address, filter it out, and leave `SymbolAddressData` pointing at `-1U`.
    - Else, update each `SymbolAddressData` to point at the first index into the vector with that address.
- Update `getMainBinarySymbolNames()`:
    - If the index is `-1U`, return itself.
    - Else, iterate through the vector starting from the saved index.

Probably not worth doing, but if there are concerns about memory overhead, this would reduce the overhead significantly.

(Maybe a simpler alternative would be to skip caching the index in the StringMap. Instead, do a `std::lower_bound()` to find the first symbol with the same address. If that first symbol is `end()` or has the wrong address, then you have the `-1U` case. This might be faster overall, depending on the workload, especially if the common case is "one symbol per address", since the vector will end up being pretty small after filtering and the binary search will be trivially fast.)


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

https://reviews.llvm.org/D121006