[PATCH] D26199: [ELF] - Implemented threaded --build-id computation

[Rui Ueyama via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/Config.h:147
   uint64_t ZStackSize;
+  unsigned BuildIdChunkSize = 1024 * 1024;
   unsigned LtoPartitions;
----------------
There's no real benefit to make it customziable, so please remove from Config.


================
Comment at: ELF/SyntheticSections.cpp:61
 template <class ELFT>
+void BuildIdSection<ELFT>::doTree(
+    llvm::ArrayRef<uint8_t> Buf,
----------------
doTree sounds a bit weird. Maybe something like computeHash.


================
Comment at: ELF/SyntheticSections.cpp:62
+void BuildIdSection<ELFT>::doTree(
+    llvm::ArrayRef<uint8_t> Buf,
+    std::function<void(ArrayRef<uint8_t> Job, uint8_t *Dest)> Worker) {
----------------
You don't have to pass Buf because it's a class member.


================
Comment at: ELF/SyntheticSections.cpp:63
+    llvm::ArrayRef<uint8_t> Buf,
+    std::function<void(ArrayRef<uint8_t> Job, uint8_t *Dest)> Worker) {
+  size_t NumChunks = (Buf.size() - 1) / Config->BuildIdChunkSize + 1;
----------------
Please don't name Job or Worker because they sound like high level stuffs. They are in fact just an ArrayRef and a callback function.



================
Comment at: ELF/SyntheticSections.cpp:65-67
+  MutableArrayRef<uint8_t> Tree = {
+      BAlloc.template Allocate<uint8_t>(NumChunks * HashSize),
+      NumChunks * HashSize};
----------------
This is not a tree but an array.

I don't know why you are using the global allocator here. This can be a local std::vector variable, no?


================
Comment at: ELF/SyntheticSections.cpp:69
+
+  auto CallWorker = [&](size_t Id) {
+    ArrayRef<uint8_t> Part = Buf.drop_front(Id * Config->BuildIdChunkSize)
----------------
I think I don't understand the code. So, basically, what you want to do in this function is

1. Split Buf into small chunks. So we need a function, say split(), which takes an ArrayRef<uint8_t> and returns std::vector<ArrayRef<uint8_t>>.
2. Call a callback for each chunk.
3. And then compute the final hash.

What we need to do here is not too complicated, so I think you can simplify the code.


================
Comment at: ELF/SyntheticSections.cpp:96
 
-  // 64-bit xxhash
-  uint64_t Hash = xxHash64(toStringRef(Buf));
-  write64<E>(this->getOutputLoc(Buf.begin()) + 16, Hash);
+  doTree(Buf, [&](ArrayRef<uint8_t> Job, uint8_t *Dest) { DoHash(Job, Dest); });
 }
----------------
You can directly pass DoHash to the function, e.g., `doTree(Buf, DoHash)`. Dohash sounds a bit weird too, but you don't really name it in this context.

  doTree(Buf, [](...) {
    ...
  });


https://reviews.llvm.org/D26199