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

Comments

Projects
None yet
4 participants
@dr2chase
Contributor

dr2chase commented Aug 22, 2016

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

This comment has been minimized.

Member

bradfitz commented Aug 22, 2016

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

This comment has been minimized.

Contributor

dr2chase commented Aug 22, 2016

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

This comment has been minimized.

Member

bradfitz commented Aug 22, 2016

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

This comment has been minimized.

Contributor

griesemer commented Aug 22, 2016

I'm with @bradfitz.

@rasky

This comment has been minimized.

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

This comment has been minimized.

Member

bradfitz commented Aug 22, 2016

@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

This comment has been minimized.

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

This comment has been minimized.

Contributor

dr2chase commented Aug 22, 2016

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment