Wasmtime GitHub notifications bot (Apr 08 2021 at 16:49): Wasmtime GitHub notifications bot (Apr 08 2021 at 16:49):alexcrichton opened PR #2818 from faster-lookup to main:
This commit is intended to be a perf improvement for instantiation of
modules with lots of functions. Previously thelookup_shared_signature
callback was showing up quite high in profiles as part of instantiation.As some background, this callback is used to translate from a module's
SignatureIndexto aVMSharedSignatureIndexwhich the instance
stores. This callback is called for two reasons, one is to translate all
of the module's own types intoVMSharedSignatureIndexfor the purposes
ofcall_indirect(the translation of that loads from this table to
compare indices). The second reason is that aVMCallerCheckedAnyfunc
is prepared for all functions and this embeds aVMSharedSignatureIndex
inside of it.The slow part today is that the lookup callback was called
once-per-function and each lookup involved hashing a full
WasmFuncType. Albeit our hash algorithm is still Rust's default
SipHash algorithm which is quite slow, but we also shouldn't need to
re-hash each signature if we see it multiple times anyway.The fix applied in this commit is to change this lookup callback to an
enumwhere one variant is that there's a table to lookup from. This
table is aPrimaryMapwhich means that lookup is quite fast. The only
thing we need to do is to prepare the table ahead of time. Currently
this happens on the instantiation path because in my measurments the
creation of the table is quite fast compared to the rest of
instantiation. If this becomes an issue, though, we can look into
creating the table as part ofSigRegistry::register_moduleand caching
it somewhere (I'm not entirely sure where but I'm sure we can figure it
out).There's in generally not a ton of efficiency around the
SigRegistry
type. I'm hoping though that this fixes the next-lowest-hanging-fruit in
terms of performance without complicating the implementation too much. I
tried a few variants and this change seemed like the best balance
between simplicity and still a nice performance gain.Locally I measured an improvement in instantiation time for a large-ish
module by reducing the time from ~3ms to ~2.6ms per instance.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Wasmtime GitHub notifications bot (Apr 08 2021 at 17:47):alexcrichton updated PR #2818 from faster-lookup to main.
Wasmtime GitHub notifications bot (Apr 08 2021 at 18:17):fitzgen submitted PR Review.
Wasmtime GitHub notifications bot (Apr 08 2021 at 18:17):fitzgen submitted PR Review.
Wasmtime GitHub notifications bot (Apr 08 2021 at 18:17):fitzgen created PR Review Comment:
Was this showing up in profiles? It seems unrelated to the rest of the changes in this PR and the early return seems like it is necessary to avoid tripping the
assert!(old.is_none())assertion at the end of this function.
Wasmtime GitHub notifications bot (Apr 08 2021 at 18:17):fitzgen created PR Review Comment:
Rather than returning the
PrimaryMapand giving up ownership, could we keep the map onself, clear it and rebuild it on each call, and reutrn a&PrimaryMap? This way we could reuse the underlying allocation.I suppose -- at that point, though -- we might as well move this out and cache it :sparkles: somewhere :sparkles: like you mention in the OP. I guess finding that somewhere is a bit hard because
SignatureRegistrys areStore-specific, so we can't hang this map off theModule.
Wasmtime GitHub notifications bot (Apr 08 2021 at 18:17):fitzgen created PR Review Comment:
Ah okay this is where all that stuff about whether we are registering for the first time or not is handled. (FWIW, this would have been a little easier on me, as a reviewer, if it were in a separate commit)
Wasmtime GitHub notifications bot (Apr 08 2021 at 18:17):fitzgen created PR Review Comment:
Similarly confused with what's going on over here.
Wasmtime GitHub notifications bot (Apr 08 2021 at 18:45):alexcrichton submitted PR Review.
Wasmtime GitHub notifications bot (Apr 08 2021 at 18:45):alexcrichton created PR Review Comment:
Oops my bad, sorry about that! I forgot to call this out in the description as well...
Wasmtime GitHub notifications bot (Apr 08 2021 at 19:16):alexcrichton submitted PR Review.
Wasmtime GitHub notifications bot (Apr 08 2021 at 19:16):alexcrichton created PR Review Comment:
Hm good point! Given what we were discussing on chat though where this may soon move into a
EngineI think I might leave this as-is for now and we can revisit this later if this gets costly in the future (so far this isn't showing up when benchmarking instantiation for me).
Wasmtime GitHub notifications bot (Apr 08 2021 at 19:39):peterhuene submitted PR Review.
Wasmtime GitHub notifications bot (Apr 08 2021 at 19:39):peterhuene created PR Review Comment:
This was ensuring we don't register the same module more than once in the store, however, this could probably just be
if self.module(start).is_some()so it doesn't bother looking up the pc into the already registered module.
Wasmtime GitHub notifications bot (Apr 08 2021 at 19:41):peterhuene created PR Review Comment:
Oh wait, I just caught up to the changes below. Yeah, this shouldn't be necessary any more.
Wasmtime GitHub notifications bot (Apr 08 2021 at 19:41):peterhuene submitted PR Review.
Wasmtime GitHub notifications bot (Apr 08 2021 at 20:04):alexcrichton merged PR #2818.
Last updated: Nov 22 2024 at 17:03 UTC