[PATCH] D41955: [WebAssembly] Add symbol table to LLD, 1/2

Rui Ueyama via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jan 31 16:48:08 PST 2018


ruiu added inline comments.


================
Comment at: wasm/Writer.cpp:684-712
 uint32_t Writer::lookupType(const WasmSignature &Sig) {
   auto It = TypeIndices.find(Sig);
   if (It == TypeIndices.end()) {
     error("type not found: " + toString(Sig));
     return 0;
   }
   return It->second;
----------------
ncw wrote:
> sbc100 wrote:
> > ruiu wrote:
> > > This is not related to this particular patch, but since I found this in this patch, I'll write the comment here. This piece of code is alarming because it uses hash tables. In general, in lld, I'd strongly recommend not to use hash tables or anything that is more complicated than the vector, because hash tables can make the linker noticeably slower.
> > > 
> > > If you really need to use a hash table, please look it up only once for a new piece of data, just like we do in the SymbolTable. In SymbolTable, we do lookup a new symbol name in the hash table only once to obtain a pointer, and after that, we access the symbols through the pointer. We never look up the symbol table more than once for the same data to minimize the cost of hash table lookup. I designed everything in lld that way, and that is I believe one of the big reasons why lld is so fast.
> > > 
> > > I'm not sure if it is applicable to this piece of code. If you have a small number of types and look the hash table only a few times, that's perhaps fine. But if each function have a type, and you need to insert/look up a hash table for each function, that's too much. If that's the case, could you revisit this code and redesign?
> > Yes, I think we can eliminate lookupType completely once the symbol table changes land.   We can just have the TypeIndex from each symbol as we call registerType.
> In fact I remember making the same change in WasmObjectWriter (then reverting it), using a vector of bool to store whether something had been allocated rather than a DenseMap.
> 
> In LLD there are a few hash map lookups we've added I'm afraid :( In fact COMDAT is another one, I think I can rid of the hashmap LLD uses for that as well. That was my fault, before I realised hash maps were forbidden.
> 
It's not forbidden. :) For example SymbolTable is a valid use of the hash table -- I can't think of any way to avoid hash table in that class. But it is strongly discouraged because it sometimes has a significant performance impact.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D41955





More information about the llvm-commits mailing list