mgechev / revive

🔥 ~6x faster, stricter, configurable, extensible, and beautiful drop-in replacement for golint
https://revive.run
MIT License
4.69k stars 273 forks source link

should revive recommend wrapping all tight loops in an anoymous function for optimal performance, given golang #63332? #914

Closed zachgrayio closed 8 months ago

zachgrayio commented 9 months ago

https://github.com/golang/go/issues/63332#issuecomment-1745565902

😈

0-issue commented 9 months ago

This change will mostly help aggregating loops, where one computes sum(), avg(), etc where the result of one iteration is reused in the next. Just found that the problem can also be bypassed by creating a useless variable, instead of wrapping in an anonymous function. Check the use of useless variable sendsum in https://github.com/golang/go/issues/63332#issuecomment-1748585137.

Looks like a performance mantra here is to create new variables for pre-allocated variables while performing loop calculations. This can be seen from the listing at the bottom of same comment, where I compare performance of aggregation on a member of struct https://github.com/golang/go/issues/63332#issuecomment-1748585137. Not sure if Go team has a document on producing performant code...

zachgrayio commented 9 months ago

Yeah @amanvm thanks I just saw your comment over there. The useless variable trick is at least more reasonable than the redundant func wrapper, very good find.

We hand tune a ton of golang using "cool tricks" to work around the compiler and runtime, looks like this is another one to add to the list.

As far as writing performant go, there's a few third party posts around how to do this but honestly it's very far from idiomatic golang in a lot of cases so it won't ever get much official acknowledgment I don't think, but yeah that's where the community comes in imo.

This issue is only half serious, hence the super-serious description, but actually given that you found an easier way to trick the compiler here, I'm actually starting to take this more seriously, and a lint rule for these performance gotchas is sounding more and more reasonable to me