Cache Abbreviation entries after materializing them to speedup MCCAS replay #7852

rastogishubham · 2023-12-06T20:20:08Z

When looking at a trace of MCCAS replay, we noticed that a large chunk of time is spent in the function visitDIEAttrs

The reason for that is because, we materialize abbreviations every time we see a new DIE, which leads to decoding the same ULEB128 values that represent the form and attribute of a DIE for every DIE in the debug info section. We can materialize them once and cache them, which would reduce the time spent and give us some speedup.

We see some speedup as well:

MCCAS With Abbrev Caching
ninja clang 150.46s user 48.35s system 536% cpu 37.071 total

ninja clang 150.43s user 47.33s system 561% cpu 35.240 total

ninja clang 150.78s user 47.23s system 565% cpu 34.987 total

MCCAS Without Abbrev Caching
ninja clang 193.36s user 48.17s system 557% cpu 43.320 total

ninja clang 190.28s user 46.98s system 595% cpu 39.867 total

ninja clang 191.55s user 46.95s system 602% cpu 39.602 total

felipepiovezan

exciting!

llvm/lib/MCCAS/MCCASObjectV1.cpp

adrian-prantl · 2023-12-15T04:39:57Z

llvm/lib/MCCAS/MCCASObjectV1.cpp

+}
+
+Error DIEVisitor::materializeAbbrevDIE(unsigned AbbrevIdx) {
+  constexpr auto AddrSize = 8;


Where does this come from? Should it be hardcoded?

For all of our purposes the AddressSize is always 8 bytes on MachO, if you see DIEVisitor::visitDIEAttrs, the same is true there

In fact, if you see getAndSetDebugAbbrevOffsetAndSkip, we have a check to make sure that the address size is 8

if (AddressSize != 8) return createStringError( inconvertibleErrorCode(), "Address size is not 8 bytes, unsupported architecture for MCCAS!");

adrian-prantl · 2023-12-15T04:40:28Z

llvm/lib/MCCAS/MCCASObjectV1.cpp

+Error DIEVisitor::materializeAbbrevDIE(unsigned AbbrevIdx) {
+  constexpr auto AddrSize = 8;
+  constexpr auto FormParams =
+      dwarf::FormParams{4 /*Version*/, AddrSize, dwarf::DwarfFormat::DWARF32};


Is that also the version for DWARF 5?

I just realized, what happens if you use MCCAS with a dwarf 2 binary? Is there a graceful degradation path?

I guess we never really addressed the dwarf version issue.
We need to do that asap but separately

I agree with Felipe, the DWARF version is a separate issue and I will put up a separate patch for that

adrian-prantl · 2023-12-15T04:42:48Z

llvm/lib/MCCAS/MCCASObjectV1.cpp

+  }
+
+  auto AbbrevDIECacheVal =
+      AbbrevDIECache[decodeAbbrevIndexAsAbbrevSetIdx(AbbrevIdx)];


we usually use the find() method because operator[] default-constructs the value and thus silently fails if there is no cache hit.

This should be more evident once we disable the default constructor for that struct

I am confused, AbbrevDIECache is a SmallVector, not a DenseMap

Ah nevermind.

So how does this function work? It materializes all abbreviations (3299) every time an entry is needed? I must be missing something obvious...

The issue has been fixed, that was an oversight which did work when this patch was on top of the patch that switched materialization to iterative from recursive, but now I have a constructor in DIEVisitor to do the AbbrevEntryCache creation instead

felipepiovezan

Sorry, a bunch of things escaped me the first time I looked into this. Feel free to merge once you address these and Adrian's comments.
(I think the DWARF version issue should be handled separately, as it is a pre-existing problem)

llvm/lib/MCCAS/MCCASObjectV1.cpp

felipepiovezan · 2023-12-15T12:10:11Z

llvm/lib/MCCAS/MCCASObjectV1.cpp

+Error DIEVisitor::materializeAbbrevDIE(unsigned AbbrevIdx) {
+  constexpr auto AddrSize = 8;
+  constexpr auto FormParams =
+      dwarf::FormParams{4 /*Version*/, AddrSize, dwarf::DwarfFormat::DWARF32};


I guess we never really addressed the dwarf version issue.
We need to do that asap but separately

llvm/lib/MCCAS/MCCASObjectV1.cpp

felipepiovezan · 2023-12-15T12:24:11Z

llvm/lib/MCCAS/MCCASObjectV1.cpp

+  }
+
+  auto AbbrevDIECacheVal =
+      AbbrevDIECache[decodeAbbrevIndexAsAbbrevSetIdx(AbbrevIdx)];


This should be more evident once we disable the default constructor for that struct

llvm/lib/MCCAS/MCCASObjectV1.cpp

felipepiovezan

LGTM

felipepiovezan · 2023-12-18T12:10:56Z

llvm/lib/MCCAS/MCCASObjectV1.cpp

+    AbbrevEntryCache.reserve(AbbrevEntries.size());
+    for (unsigned I = 0; I < AbbrevEntries.size(); I++) {
+      if (Error E = materializeAbbrevDIE(encodeAbbrevIndex(I)))
+        report_fatal_error(std::move(E));


Note that we are crashing the compiler on invalid input.
I don't remember how this used to be done in previous iterations of this PR, but we could (maybe in a separate PR) move this out of DIEVisitor entirely; the DIEVisitor could take the already-cached abbreviations

Yes, once we move to the flat patch, we can do this materialization in visitDIERef

llvm/lib/MCCAS/MCCASObjectV1.cpp

rastogishubham · 2023-12-19T21:42:49Z

@swift-ci please test llvm

This patch caches abbreviation entries after materializing them onces to avoid rematerializing the same abbreviation entry for every DIE, thus speeding up MCCAS replay

Cherry-pick PR #7852 into stable/20230725

rastogishubham requested review from felipepiovezan and adrian-prantl and removed request for felipepiovezan December 6, 2023 22:49

rastogishubham force-pushed the CachingMCCAS branch 4 times, most recently from c515933 to 53fe5f9 Compare December 7, 2023 03:34

felipepiovezan reviewed Dec 8, 2023

View reviewed changes

llvm/lib/MCCAS/MCCASObjectV1.cpp Outdated Show resolved Hide resolved

llvm/lib/MCCAS/MCCASObjectV1.cpp Outdated Show resolved Hide resolved

llvm/lib/MCCAS/MCCASObjectV1.cpp Show resolved Hide resolved

llvm/lib/MCCAS/MCCASObjectV1.cpp Outdated Show resolved Hide resolved

rastogishubham force-pushed the CachingMCCAS branch 2 times, most recently from 2158b92 to 53fe5f9 Compare December 9, 2023 21:41

adrian-prantl reviewed Dec 13, 2023

View reviewed changes

llvm/lib/MCCAS/MCCASObjectV1.cpp Outdated Show resolved Hide resolved

rastogishubham force-pushed the CachingMCCAS branch from 53fe5f9 to 7922368 Compare December 14, 2023 01:41

rastogishubham requested review from adrian-prantl and felipepiovezan December 14, 2023 01:42

adrian-prantl reviewed Dec 15, 2023

View reviewed changes

felipepiovezan approved these changes Dec 15, 2023

View reviewed changes

adrian-prantl approved these changes Dec 15, 2023

View reviewed changes

rastogishubham force-pushed the CachingMCCAS branch 3 times, most recently from 6b8bb0c to ec9ed7f Compare December 18, 2023 09:05

rastogishubham requested review from adrian-prantl and felipepiovezan December 18, 2023 09:10

felipepiovezan approved these changes Dec 18, 2023

View reviewed changes

rastogishubham force-pushed the CachingMCCAS branch from ec9ed7f to 5dab406 Compare December 19, 2023 02:44

adrian-prantl approved these changes Dec 19, 2023

View reviewed changes

rastogishubham force-pushed the CachingMCCAS branch from 5dab406 to 6fcca53 Compare December 19, 2023 21:47

Introduce AbbrevContent and AbbrevEntry

90eb045

Cache AbbrevEntries after materializing

4e9c0dc

This patch caches abbreviation entries after materializing them onces to avoid rematerializing the same abbreviation entry for every DIE, thus speeding up MCCAS replay

rastogishubham force-pushed the CachingMCCAS branch from 6fcca53 to 4e9c0dc Compare December 19, 2023 23:01

rastogishubham merged commit 09b24c2 into swiftlang:next Dec 19, 2023

This was referenced Dec 20, 2023

[NFC] Remove DIEVisitor constructor and move AbbrevCache population #7911

Merged

Make FormParams have the correct DWARF version. #7912

Merged

rastogishubham added a commit that referenced this pull request Dec 20, 2023

Merge pull request #7909 from rastogishubham/CachingMCCASStable

1312409

Cherry-pick PR #7852 into stable/20230725

Cache Abbreviation entries after materializing them to speedup MCCAS replay #7852

Cache Abbreviation entries after materializing them to speedup MCCAS replay #7852

Uh oh!

Conversation

rastogishubham commented Dec 6, 2023

Uh oh!

felipepiovezan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felipepiovezan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

felipepiovezan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rastogishubham commented Dec 19, 2023

Uh oh!

Uh oh!