ms-pdb: Precompute deserialized symbols for pdb once and then use it in sort instead of doing it in the comparison function

[alex.telishev]

Summary

computeAddrMap function calls std::stable_sort with a comparison function that computes deserialized symbols every time its called. In the result deserializeAs<PublicSym32> is called 20-30 times for every symbol. It's much faster to calculate it beforehand and pass pointer to it to the comparison function.

[alex.telishev]

Fix style

[zturner]

Looks ok to me but I'll defer to ruiu@ for the final word since I'm not as familiar with this area.

[zturner]

Err wait, you uploaded to the wrong patch. This was supposed to be the PDB patch :) That one looks good. Can you re-upload this to the ICF patch?

[alex.telishev]

Oops, sorry. Re-uploaded it to the right patch

llvm/lib/DebugInfo/PDB/Native/GSIStreamBuilder.cpp

Error GSIStreamBuilder::finalizeMsfLayout() {
 
   Idx = Msf.addStream(RecordBytes);
   if (!Idx)
     return Idx.takeError();
   RecordStreamIdx = *Idx;
   return Error::success();
 }
 
-static bool comparePubSymByAddrAndName(const CVSymbol *LS, const CVSymbol *RS) {
-  assert(LS->kind() == SymbolKind::S_PUB32);
-  assert(RS->kind() == SymbolKind::S_PUB32);
-
-  PublicSym32 PSL =
-      cantFail(SymbolDeserializer::deserializeAs<PublicSym32>(*LS));
-  PublicSym32 PSR =
-      cantFail(SymbolDeserializer::deserializeAs<PublicSym32>(*RS));
-
-  if (PSL.Segment != PSR.Segment)
-    return PSL.Segment < PSR.Segment;
-  if (PSL.Offset != PSR.Offset)
-    return PSL.Offset < PSR.Offset;
+static bool comparePubSymByAddrAndName(const std::pair<const CVSymbol *, const  PublicSym32 *> &LS, const std::pair<const CVSymbol *, const PublicSym32 *> &RS)
+{
+  if (LS.second->Segment != RS.second->Segment)
+    return LS.second->Segment < RS.second->Segment;
+  if (LS.second->Offset != RS.second->Offset)
+    return LS.second->Offset < RS.second->Offset;
 
-  return PSL.Name < PSR.Name;
+  return LS.second->Name < RS.second->Name;
 }
 
 /// Compute the address map. The address map is an array of symbol offsets
 /// sorted so that it can be binary searched by address.
 static std::vector<ulittle32_t> computeAddrMap(ArrayRef<CVSymbol> Records) {
   // Make a vector of pointers to the symbols so we can sort it by address.
   // Also gather the symbol offsets while we're at it.
-  std::vector<const CVSymbol *> PublicsByAddr;
+
+  std::vector<PublicSym32> DeserializedPublics;
+  std::vector<std::pair<const CVSymbol *, const PublicSym32 *>> PublicsByAddr;
   std::vector<uint32_t> SymOffsets;
+  DeserializedPublics.reserve (Records.size());
   PublicsByAddr.reserve(Records.size());
+  SymOffsets.reserve(Records.size());
+
   uint32_t SymOffset = 0;
   for (const CVSymbol &Sym : Records) {
-    PublicsByAddr.push_back(&Sym);
+    assert(Sym.kind() == SymbolKind::S_PUB32);
+    DeserializedPublics.push_back (cantFail (SymbolDeserializer::deserializeAs<PublicSym32>(Sym)));
+    PublicsByAddr.emplace_back (&Sym, &DeserializedPublics.back ());
     SymOffsets.push_back(SymOffset);
     SymOffset += Sym.length();
   }
   std::stable_sort(PublicsByAddr.begin(), PublicsByAddr.end(),
                    comparePubSymByAddrAndName);
 
   // Fill in the symbol offsets in the appropriate order.
   std::vector<ulittle32_t> AddrMap;
   AddrMap.reserve(Records.size());
-  for (const CVSymbol *Sym : PublicsByAddr) {
-    ptrdiff_t Idx = std::distance(Records.data(), Sym);
+  for (auto &Sym : PublicsByAddr) {
+    ptrdiff_t Idx = std::distance(Records.data(), Sym.first);
     assert(Idx >= 0 && size_t(Idx) < Records.size());
     AddrMap.push_back(ulittle32_t(SymOffsets[Idx]));
   }
   return AddrMap;
 }
 
 uint32_t GSIStreamBuilder::getPublicsStreamIndex() const {
   return PSH->StreamIndex;