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

[George Rimar via llvm-commits]

grimar added inline comments.


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


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


================
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) {
----------------
ruiu wrote:
> You don't have to pass Buf because it's a class member.
I have to. Argument Buf was a data to hash, it is not the same that member which is a section content.
I renamed argument to Data to avoid confusion (actually I did not notice the member with the same name when wrote it).


================
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;
----------------
ruiu wrote:
> 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.
> 
Ok, done.


================
Comment at: ELF/SyntheticSections.cpp:65-67
+  MutableArrayRef<uint8_t> Tree = {
+      BAlloc.template Allocate<uint8_t>(NumChunks * HashSize),
+      NumChunks * HashSize};
----------------
ruiu wrote:
> 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?
I expected that might be a bit faster. Removed.


================
Comment at: ELF/SyntheticSections.cpp:69
+
+  auto CallWorker = [&](size_t Id) {
+    ArrayRef<uint8_t> Part = Buf.drop_front(Id * Config->BuildIdChunkSize)
----------------
ruiu wrote:
> 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.
Reimplemented.


================
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); });
 }
----------------
ruiu wrote:
> 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, [](...) {
>     ...
>   });
Done.


https://reviews.llvm.org/D26199