-
Notifications
You must be signed in to change notification settings - Fork 830
Emit optimal-size LEBs in section/subsection/function body sizes #1128
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
Conversation
…tead of preallocating 5 bytes
src/wasm-binary.h
Outdated
|
|
||
| enum { | ||
| // the maximum amount of bytes we emit per LEB | ||
| MAX_LEB32_BYTES = 5 |
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.
Our existing style for enums mostly uses Capital format rather than all caps.
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.
Done.
src/wasm/wasm-binary.cpp
Outdated
| if (sizeFieldSize != MAX_LEB32_BYTES) { | ||
| // we can save some room, nice | ||
| assert(sizeFieldSize < MAX_LEB32_BYTES); | ||
| std::move(&o[start + MAX_LEB32_BYTES], &o[start + MAX_LEB32_BYTES + size], &o[start + sizeFieldSize]); |
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.
is this supposed to be std::memmove?
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.
Nope, there's an algorithm for it now too: http://en.cppreference.com/w/cpp/algorithm/move
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.
Though in this case it probably could be memmove; I wouldn't be surprised if it compiles to the same code though
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.
Yeah, it could turn into a memmove. Seems cleaner to write move though, imo.
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.
Oh, hm. OTOH, this should be std::move_backward actually since the regions overlap.
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.
I find this stuff confusing so I'm not sure, but this is moving to the left (we get rid of extra padding, we shrink), which these docs say should be move,
When moving overlapping ranges, std::move is appropriate when moving to the left (beginning of the destination range is outside the source range) while std::move_backward is appropriate when moving to the right (end of the destination range is outside the source range).
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.
Oh, right. Never mind. :-)
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 std::move is fine, I just didn't know about it :). Let's #include <algorithm> at the top of the header though.
dschuff
left a comment
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.
LGTM with the extra include.
Instead of preallocating 5 bytes, emit the minimal bytes and move the code back.
The extra copying makes us a tiny bit slower to write (less than a quarter of a percent). But larger effect in code size than I thought, about 1% smaller. I guess it's because there are often small functions, and each got an extra 4 bytes earlier.
Fixes #1121.