-
Notifications
You must be signed in to change notification settings - Fork 954
reflect: embed internal/reflectlite types #4787
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
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 have not done a full review yet.
I do see that this approach results in a much larger binary size increase than #4774. I wonder why? Could be because of Seq/Seq2 but hard to say without testing.
@aykevl thanks for the review. What’s the best way to benchmark binary size changes? |
It does not? Here is the output of the "Binary size difference" check of this PR (for some reason it didn't create a comment, but it's in the "Calculate size diff" step):
Whereas #4774 looks like this:
26796 vs 4060 is a significant difference. And I wonder why that is. (It also says 0.00% but I believe that number is calculated wrong).
See above. It runs |
oops I read it incorrectly it seems. |
Is this a difference of 26K vs 4K? Am I reading that right? Is the last line cumulative? The proceeding lines imply a size diff of ~1% per item. |
PR #4774 has this result at present: The This PR has this result: The |
There's also something going on with that check. Especially https://github.com/tinygo-org/tinygo/actions/runs/13809326310/job/38627052954?pr=4774#step:17:29 stops way too early, probably as a result of a bug (fixed in tinygo-org/drivers#746 but not yet in the default branch). So that size is certainly not correct. |
EDIT: might have messed up tests below, see #4774 (comment) EDIT 2: updated the tests. With #4774 fixed, the difference is not nearly as big: 5376 vs 16320 total increase (#4774 is still smaller though). Finished #4774 including the change to the runtime package (but it doesn't include the Seq changes). For #4774, on my system I get a total change of 5376 bytes. A bunch of tests add ~150 bytes, but there are also a few that are reduced in size slightly. Also, RAM usage is down by 8-16 bytes in some cases. Details: PR #4774 diff (click to expand)
For a fair comparson against this PR, I rebased this PR against the dev branch (04c7057) and removed the Seq related bits (since the Seq bits add code size). The result is an increase of 16320 total bytes. There are a bunch of tests that add ~400 bytes, while there are no decreases in code size. Details: This PR diff (click to expand)
The drivers repo commit I used for testing against is 156d6e7c9ce473be185e1554c9da3c2983e6e26a. |
Thanks. I suspect the additional code size comes from methods on Type or Value that are now in reflectlite. It's possible to move functions into package reflect that aren't required for consumers of reflectlite. Thoughts? |
Ideally |
e331819
to
25ddc3c
Compare
Seems like maintainability wins this round, since the size increase is not much more than the alternative. Perhaps in a future PR some small decreases can be found. In any case it is not worth holding back the needed fixes unblocked by this PR. Now going to merge unless any objections. |
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.
Looks good to me.
src/internal/reflectlite/strconv.go
Outdated
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.
The file src/reflect/strconv.go can be removed.
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.
Removed, rebased, and pushed.
25ddc3c
to
58c8c81
Compare
… to package reflect
…ds on reflect.Value
Implementation of golang/go#71905 194696f1d1f6e5609f96d0fb0192595e7e0f5b90
58c8c81
to
ad18ffa
Compare
…er.Reset Fixes tinygo-org#4790. Depends on tinygo-org#4787 (merge that first).
OK, @deadprogram ready to go! |
…er.Reset Fixes tinygo-org#4790. Depends on tinygo-org#4787 (merge that first).
This PR is an alternative to #4774, using type aliases and struct embedding in lieu of
go:linkname
.reflect
now depends on packageinternal/reflectlite
, reversing the existing dependency.iter.Seq
methods toreflect
.runtime
andreflect
introduced via packageiter
in Go 1.24.Note
An alternative to this PR could implement the common functionality in
reflect
andreflectlite
in a new, third package (e.g.internal/tinyreflect
).