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

cmd/compile: should recognize binary package and intrinsify it where possible #16832

Open
dr2chase opened this issue Aug 22, 2016 · 8 comments
Open
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Performance
Milestone

Comments

@dr2chase
Copy link
Contributor

The compiler should recognize calls to binary.Reader and binary.Writer where the byte order and type of data interface{} are compile-time constants, and based on that "do the right thing".

One candidate for "the right thing" is, for target/source types that contain no padding and for the native byte order, alias a byte slice to the storage, and pass that to the read or write method. Further enhancements could deal with padding and mismatched native/requested byte order.

@bradfitz
Copy link
Contributor

We've tried not to specialize the compiler against "regular" packages in the standard library.

Taking regular packages from std and moving them to github or a project's vendor directory should not affect their performance.

Sometimes we specialize for special packages (reflect, runtime, maybe even time), but not "encoding/binary".

@dr2chase
Copy link
Contributor Author

I understand the reasoning, but the result of this is, as I understand it, that the binary package is (regarded as) slow and ends up unused, with hand-crafted (sometimes unsafe) code used in its place.

@bradfitz
Copy link
Contributor

To be clear, I'm only opposed to special-casing on the (package name, function name) pair alone. It would be acceptable to make the compiler recognize the structure of the function and special-cases on that instead.

@bradfitz bradfitz added this to the Unplanned milestone Aug 22, 2016
@griesemer
Copy link
Contributor

I'm with @bradfitz.

@rasky
Copy link
Member

rasky commented Aug 22, 2016

What about adding a (cached) reflect.HasPadding to detect structs that contain no pointers and have no padding? That should allow to implement the optimization in pure Go in the package, without any special casing in the compiler.

@bradfitz
Copy link
Contributor

@rasky, this bug isn't about structs. @dr2chase mentioned compile-time constants (and structs can't be constant). Even with variables, it'd be nice to eliminate the call overhead entirely ("intrinsify").

@rasky
Copy link
Member

rasky commented Aug 22, 2016

Uh? He wrote:

... and type of data interface{} are compile-time constants

Anyway, I do wonder how a compiler can reliably match a function as complex as encoding/binary and intrinsify it. In my experience, encoding/binary is very slow when it has to go through struct fields with reflection, and my suggestion would allow to skip that big overhead almost altogether (assuming reflect.HasPadding is cached and/or computed at compile-time). Call overhead could then be removed with inlining.

@dr2chase
Copy link
Contributor Author

Another possibility might be to create a "BinaryReader" and "BinaryWriter" that would be created for objects of a specific type and data with a particular endianness, and used repeatedly afterwards, in the same way that regular expressions can be compiled into matchers.

And the obviously easy special case is an obviously easy special case.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Performance
Projects
None yet
Development

No branches or pull requests

5 participants