Use direct pointers as Type IDs #2745
Conversation
Instead of using indices into the global interned type table. This means that a lock is *never* needed to access an expanded Type. The Type lock is now only acquired when a complex Type is created. On a real-world wasm2js workload this improves wall clock time by 23% on my machine with 72 cores and makes traffic on the Type lock entirely insignificant. **Before** 72 cores real 0m6.914s user 184.014s sys 0m3.995s 1 core real 0m25.903s user 0m25.658s sys 0m0.253s **After** 72 cores real 5.349s user 70.309s sys 9.691s 1 core real 25.859s user 25.615s sys 0.253s
|
Ah, the tracing in the C API needs to be redone because tuple Type IDs (aka the locations in memory of their canonical Type vectors) are not stable between the traced program and the program emitted as the trace. |
src/wasm/wasm-type.cpp
Outdated
| } | ||
| id = typeLists.size(); | ||
| typeLists.push_back(std::make_unique<std::vector<Type>>(types)); | ||
| auto* vec = new std::vector<Type>(types); |
There was a problem hiding this comment.
NEVER! The canonical type vectors live for the length of the process, just like they did before.
There was a problem hiding this comment.
That sounds ok, but before it was making a unique ptr, so it was cleaned up, wasn't it?
There was a problem hiding this comment.
Yes, the unique_ptr was cleaned up, but only when global destructors run at the end of the program. I could make this a unique_ptr again but I'm not sure whether that has any value.
There was a problem hiding this comment.
It would avoid false positives with leak detection tools. I'm unsure myself if that is worth the cost.
|
@kripken The latest commit adds unique_ptrs to avoid false leaks and switches to using a normal mutex for simplicity. I think this should be good to go now. |
src/wasm/wasm-type.cpp
Outdated
| // Track unique_ptrs for constructed types to avoid leaks | ||
| std::vector<std::unique_ptr<std::vector<Type>>> constructedTypes; | ||
|
|
||
| // Maps from type vectors to the canonical Type IS |
| } | ||
| if (types.size() == 1) { | ||
| *this = types[0]; | ||
| return; | ||
| } |
There was a problem hiding this comment.
I think I don't understand how this all works yet (maybe some overview in types.h?). Here in init we take a lock on anything that doesn't have 0 or 1 elements. Are we somehow not calling init as much? If so I missed that part (or something else).
There was a problem hiding this comment.
The problem was never that we called init a lot, it was that we called expand a lot and it acquired the lock. Now expand never acquires the lock. I'll add documentation about how the IDs work in the header.
|
This seems to have hit an error on the mac CI after landing: https://travis-ci.org/github/WebAssembly/binaryen/jobs/674594365 |
Instead of using indices into the global interned type table. This
means that a lock is never needed to access an expanded Type. The
Type lock is now only acquired when a complex Type is created. On a
real-world wasm2js workload this improves wall clock time by 23% on my
machine with 72 cores and makes traffic on the Type lock entirely
insignificant.
Before
72 cores
real 6.914s
user 184.014s
sys 3.995s
1 core
real 25.903s
user 25.658s
sys 0.253s
After
72 cores
real 5.349s
user 70.309s
sys 9.691s
1 core
real 25.859s
user 25.615s
sys 0.253s