-
Notifications
You must be signed in to change notification settings - Fork 696
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimize interpreter and Opcode::FromCode
#910
Conversation
`Opcode::FromCode` calculated the opcode given a prefix/code pair by using lower_bound over the list of all `OpcodeInfo`s. This was happening for every instruction, which is incredibly slow. Since the interpreter's format is internal only, we can use any encoding we want, so it's simpler and faster to use the `Opcode::Enum` directly without calling `Opcode::FromCode`. `Opcode::FromCode` is also used when reading a binary file, so it should be optimized anyway. Instead of using the `infos_` table, which is indexed by the opcode's `enum_` value, we create a new statically-defined table that maps from prefix-code pair to its enum value. Unfortunately, this can't be done easily in C++ because it does not currently support designated array initializers, so this table is created in a C file instead, `opcode-code-table.c`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally lgtm
src/opcode-code-table.c
Outdated
Invalid, | ||
} WabtOpcodeEnum; | ||
|
||
_Static_assert(Invalid <= 65536, "Too many opcodes"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe static const size_t kNumOpcodes = 65536;
in opcode.h
Especially because the extern declaration there needs to match
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit tricky since I can't include opcode.h here (since it's C++). So I'd have to make a C header and include it there. I was trying to avoid adding a new file, but there's no reason to do so, really.
src/opcode-code-table.c
Outdated
uint32_t WabtOpcodeCodeTable[65536] = { | ||
#define WABT_OPCODE(rtype, type1, type2, type3, mem_size, prefix, code, Name, \ | ||
text) \ | ||
[(prefix << 8) + code] = Name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file has to be compiled as C for this to work, right?
Not gonna lie, this is really neat. Meaning both "nifty" and "clean"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, it's a C99-only feature (until C++20, I think). It works, but I dunno, I thought about doing this before and thought it was kind of gross. :-) But the alternatives are not great either, like building the table at runtime, generating it via script, or writing it out manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a constexpr
static initializer? Should be doable even with C++11 constexprs. Not sure if it's less-gross though
Opcode::FromCode
calculated the opcode given a prefix/code pair byusing lower_bound over the list of all
OpcodeInfo
s. This was happeningfor every instruction, which is incredibly slow.
Since the interpreter's format is internal only, we can use any encoding
we want, so it's simpler and faster to use the
Opcode::Enum
directlywithout calling
Opcode::FromCode
.Opcode::FromCode
is also used when reading a binary file, so it shouldbe optimized anyway. Instead of using the
infos_
table, which isindexed by the opcode's
enum_
value, we create a newstatically-defined table that maps from prefix-code pair to its enum
value.
Unfortunately, this can't be done easily in C++ because it does not
currently support designated array initializers, so this table is
created in a C file instead,
opcode-code-table.c
.