Skip to content
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

compiler,reflect: make field offsets varints #3689

Merged
merged 4 commits into from May 16, 2023

Conversation

dgryski
Copy link
Member

@dgryski dgryski commented Apr 27, 2023

Fixes #3686

@dgryski
Copy link
Member Author

dgryski commented Apr 27, 2023

Impact on a production service built with -target=wasi -no-debug:

-rwxr-xr-x  1 dgryski  staff  2751357 27 Apr 09:25 service.dev*
-rwxr-xr-x  1 dgryski  staff  2750656 27 Apr 09:18 service.field-varint*

@dgryski dgryski requested a review from aykevl April 27, 2023 16:32
@deadprogram
Copy link
Member

@dgryski size changed so CI failing.

@aykevl
Copy link
Member

aykevl commented Apr 28, 2023

That's a much smaller effect than I expected, but I'll test the drivers repo too (hence why I'm working on #3687 to make this easier).

@deadprogram
Copy link
Member

@aykevl looking good to merge?

@aykevl
Copy link
Member

aykevl commented May 4, 2023

I'll try to take a look soon.

@aykevl
Copy link
Member

aykevl commented May 4, 2023

Did a quick check using the drivers smoke tests, and this does indeed result in a small binary size reduction (overall reduction of 0.21%) with a few exceptions where it increases binary size. So from that perspective, this PR looks good. I'll look at the implementation next.

(When #3687 is in, checking this will become a ton easier as I don't have to run these tests manually).

EDIT: 0.21% is actually really good, as #3656 added added 0.36% in binary size. That means that the sum increase is only 0.15% - less than half of the original increase.

Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, with some suggestions how the implementation could be slightly more efficient.

src/reflect/type.go Outdated Show resolved Hide resolved
src/runtime/interface.go Outdated Show resolved Hide resolved
Comment on lines +681 to +682
offset, lenOffs := uvarint32(unsafe.Slice((*byte)(data), maxVarintLen32))
data = unsafe.Add(data, lenOffs)
Copy link
Member

@aykevl aykevl May 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing to consider (although that might go a bit too far) is to not convert to a slice but work on raw pointers instead. That's a bit more dangerous, but may reduce binary size a little bit.

@deadprogram
Copy link
Member

@dgryski not sure if you saw @aykevl comments on this PR, but looked like pretty small changes then we can merge this one.

@deadprogram deadprogram added this to the v0.28.0 milestone May 9, 2023
@dgryski
Copy link
Member Author

dgryski commented May 15, 2023

PTAL

@deadprogram
Copy link
Member

The size reduction is working as expected, and all the immediate feedback has been addressed, so now merging. Thank you very much for working on this @dgryski and for the helpful review @aykevl

@deadprogram deadprogram merged commit d256804 into tinygo-org:dev May 16, 2023
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants