Manual byte-by-byte copy instead of binary.LittleEndian func

[diamondo25]

I was wondering why sfixed32 and sfixed64 are not using the binary.LittleEndian; the ASM output is optimized a lot.

[awalterschulze]

I can't remember, you are welcome to edit the marshal plugin and try to run the tests. If they pass you are welcome to make a pull request. I will be really interested in a Benchmark as well. I would have tested this idea out myself, but I am at a conference.

…

On Thu, 7 Sep 2017, 11:37 Erwin Oegema, ***@***.***> wrote: I was wondering why sfixed32 and sfixed64 are not using the binary.LittleEndian; the ASM output is optimized a lot. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#333>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvsLTAwFAcV5QyneS5qYC73D9wjfCT3ks5sf8dsgaJpZM4PPn2d> .

[awalterschulze]

It seems that binary.LittleEndian does an early bounds check and that is what optimizes the asm output
https://golang.org/src/encoding/binary/binary.go?s=1176:1371#L82
Is that what you were talking about?
The rest of the code looks the same and the tests work when I replace the implementation.

But I think it this is the case then we can use the same trick in more places.

I can figure it out, but its easier if you tell me, how did you discover the optimized asm output?
Because then I can try to find more places that can be optimized.

[diamondo25]

You can run 'go compile -S file.go' to get the output for the current arch. Basically, on AMD64 it showed literally every step (load byte, set byte, increase offset) and at the end the bounds check. binary.LittleEndian generates the corresponding mov instruction to copy the block. (Movq with 64-bit elements). I have been working on getting the bencmark and other tools working. Due to some missing info (t.go) and other outdated stuff, it doesnt work. I also found an error in the generator of gogo that causes lots of files to be indented by default, generating huge files. I think i can make a PR with the fixes for these things, at least. Op 9 sep. 2017 10:39 a.m. schreef Walter Schulze <notifications@github.com>: It seems that binary.LittleEndian does an early bounds check and that is what optimizes the asm output https://golang.org/src/encoding/binary/binary.go?s=1176:1371#L82 Is that what you were talking about? The rest of the code looks the same and the tests work when I replace the implementation. But I think it this is the case then we can use the same trick in more places. I can figure it out, but its easier if you tell me, how did you discover the optimized asm output? Because then I can try to find more places that can be optimized. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#333 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AACamVw5nRLa15u8nPOx7dz4YqTleVR-ks5sgk7bgaJpZM4PPn2d>.

[awalterschulze]

Ok I am going to assume you are working on this :) It is much appreciated.

It would be nice if we can also compare the binary.LittleEndian with the unsafe version:

*(*int64)(unsafe.Pointer(&dAtA[i])) = v

Because then we can hopefully remove the need for the unsafe marshaler.

[diamondo25]

I already notice that some test was checking for bigendianness, which would make it fail. Not really a good experience, to be honest. Op 9 sep. 2017 1:55 p.m. schreef Walter Schulze <notifications@github.com>: Ok I am going to assume you are working on this :) It is much appreciated. It would be nice if we can also compare the binary.LittleEndian with the unsafe version: *(*int64)(unsafe.Pointer(&dAtA[i])) = v — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#333 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AACambUXNwWCOE-zZQyhJXtUGqS5msE3ks5sgnylgaJpZM4PPn2d>.

[awalterschulze]

Yes I agree. I would really like to remove the use of unsafe.

[awalterschulze]

Please let me know if you don't have the time to do this.
I would prefer to get another contributor and I actually very busy, but if you don't have time, I think this change is important enough that I will spend time on it.

[awalterschulze]

hello @diamondo25 ?

[diamondo25]

I am sorry. I have no time to work on this. There is a lot of code that is running on precompiled code, so its hard to exclude it. I will create a branch with all the code i have today. Op 3 okt. 2017 1:07 p.m. schreef Walter Schulze <notifications@github.com>: hello @diamondo25<https://github.com/diamondo25> ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#333 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AACames2Xtq6m1aKq0sFB7nUwGJuMrnlks5sohVVgaJpZM4PPn2d>.

[awalterschulze]

No problem, just checking. Then I'll take over :)

[diamondo25]

#339

[awalterschulze]

I started with an intial comparison
https://github.com/awalterschulze/gorangecheck

The range check hint seems to require a constant.
Unsafe is still a little faster than the range checking hint, but by very little.
I think it might be time to remove unsafe and start using math and constant range check hints where we can :)

[awalterschulze]

I think its done
#343

[awalterschulze]

done