Skip to content

[lld-macho,BalancedPartition] Simplify relocation hash and avoid xxHash #121729

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 16, 2025
Merged

[lld-macho,BalancedPartition] Simplify relocation hash and avoid xxHash #121729

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
671e163
[𝘀𝗽𝗿] initial version
MaskRay Jan 6, 2025
3d4e558
change tail hashing scheme
MaskRay Jan 7, 2025
86bd9d9
.
MaskRay Jan 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 20 additions & 18 deletions lld/MachO/BPSectionOrderer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@
#include "Symbols.h"
#include "lld/Common/BPSectionOrdererBase.h"
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/StableHashing.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Endian.h"
#include "llvm/Support/xxhash.h"

namespace lld::macho {

Expand Down Expand Up @@ -90,23 +93,24 @@ class BPSectionMacho : public BPSectionBase {
&sectionToIdx) const override {
constexpr unsigned windowSize = 4;

// Calculate content hashes
size_t dataSize = isec->data.size();
for (size_t i = 0; i < dataSize; i++) {
auto window = isec->data.drop_front(i).take_front(windowSize);
hashes.push_back(xxHash64(window));
}
// Calculate content hashes: k-mers and the last k-1 bytes.
ArrayRef<uint8_t> data = isec->data;
if (data.size() >= windowSize)
for (size_t i = 0; i <= data.size() - windowSize; ++i)
hashes.push_back(llvm::support::endian::read32le(data.data() + i));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we using read32le() here and xxh3_64bits() for relocations below? As I understand, read32le() only works here because the window size is exactly 4. I chose this window size because it gave the best results on a few binaries, but other window sizes could work better for other scenarios. If we use xxh3_64bits() in both cases, we are free to change windowSize.

Copy link
Member Author

@MaskRay MaskRay Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this is weird. xxh3_64bits(window) for relocation hashing is just because it's easy: no need to handle the shorter-than-4-bytes case.

Hmmm. Reloc::length is actually a logarithm field. For Mach-O arm64, the relocation offsets are aligned to start of the instruction. Shall we compute one single hash for a relocation? I guess the sliding window doesn't help, but happy to be proven wrong.

Copy link
Contributor

@Colibrow Colibrow Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

file size gzipped size
libsample.so 3,181,560 1,487,043
libsample.so_xxh3 3,181,544 1,486,208

It seems good although the change is minor. The object file size change confused me. Do you have any ideas?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The uncompressed size could change due to alignment and changes in the unwind info section. You can use bloaty to verify this. IIRC [TEXT] will show alignment changes, but that isn't well documented.

Hmmm. Reloc::length is actually a logarithm field. For Mach-O arm64, the relocation offsets are aligned to start of the instruction. Shall we compute one single hash for a relocation? I guess the sliding window doesn't help, but happy to be proven wrong.

I settled on the current implementation by trying many different hashing strategies. I got the best results by hashing a sliding window for relocations and the section data. I'm open to changing this if we run experiments to confirm there is no regression. For now, I think those more aggressive changes should be a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather use xxh3_64bits() here which allows for more flexibility and consistency

for (uint8_t byte : data.take_back(windowSize - 1))
hashes.push_back(byte);

// Calculate relocation hashes
for (const auto &r : isec->relocs) {
if (r.length == 0 || r.referent.isNull() || r.offset >= isec->data.size())
if (r.length == 0 || r.referent.isNull() || r.offset >= data.size())
continue;

uint64_t relocHash = getRelocHash(r, sectionToIdx);
uint32_t start = (r.offset < windowSize) ? 0 : r.offset - windowSize + 1;
for (uint32_t i = start; i < r.offset + r.length; i++) {
auto window = isec->data.drop_front(i).take_front(windowSize);
hashes.push_back(xxHash64(window) + relocHash);
auto window = data.drop_front(i).take_front(windowSize);
hashes.push_back(xxh3_64bits(window) ^ relocHash);
}
}

Expand All @@ -124,19 +128,17 @@ class BPSectionMacho : public BPSectionBase {
std::optional<uint64_t> sectionIdx;
if (auto it = sectionToIdx.find(isec); it != sectionToIdx.end())
sectionIdx = it->second;
std::string kind;
uint64_t kind = -1, value = 0;
if (isec)
kind = ("Section " + Twine(isec->kind())).str();
kind = uint64_t(isec->kind());

if (auto *sym = reloc.referent.dyn_cast<Symbol *>()) {
kind += (" Symbol " + Twine(sym->kind())).str();
if (auto *d = llvm::dyn_cast<Defined>(sym)) {
return BPSectionBase::getRelocHash(kind, sectionIdx.value_or(0),
d->value, reloc.addend);
}
kind = (kind << 8) | uint8_t(sym->kind());
if (auto *d = llvm::dyn_cast<Defined>(sym))
value = d->value;
}
return BPSectionBase::getRelocHash(kind, sectionIdx.value_or(0), 0,
reloc.addend);
return llvm::stable_hash_combine(kind, sectionIdx.value_or(0), value,
reloc.addend);
}
};

Expand Down
9 changes: 0 additions & 9 deletions lld/include/lld/Common/BPSectionOrdererBase.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/Twine.h"
#include "llvm/Support/xxhash.h"
#include <memory>
#include <optional>

Expand Down Expand Up @@ -56,14 +55,6 @@ class BPSectionBase {
return P1;
}

static uint64_t getRelocHash(llvm::StringRef kind, uint64_t sectionIdx,
uint64_t offset, uint64_t addend) {
return llvm::xxHash64((kind + ": " + llvm::Twine::utohexstr(sectionIdx) +
" + " + llvm::Twine::utohexstr(offset) + " + " +
llvm::Twine::utohexstr(addend))
.str());
}

/// Reorders sections using balanced partitioning algorithm based on profile
/// data.
static llvm::DenseMap<const BPSectionBase *, size_t>
Expand Down
Loading