Eliminate duplicate logical right shifts #582

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@jbowes
Contributor
jbowes commented Feb 3, 2017

When doing an logical right shift, there's no need to append another
zero bit logical right shift to coerce to 32 bit unsigned.

@jbowes jbowes Eliminate duplicate logical right shifts
When doing an logical right shift, there's no need to append another
zero bit logical right shift to coerce to 32 bit unsigned.
32eb678
@shurcooL
Member
shurcooL commented Feb 3, 2017

Interesting, thanks for the PR.

It's strange that CI isn't running on it... @neelance, any idea why? Circle CI seems to be still on for this project.

Have you tried running any benchmarks? I understand this is meant to be an optimization, so it'd be helpful to have some numbers in order to understand the change better.

It would also be helpful to run Go1 benchmarks before and after, to see how widely applicable this optimization is. See #573, it also includes directions for running the benchmarks.

@shurcooL
Member

I ran go1 benchmarks here too, this is the result:

benchmark                          old ns/op       new ns/op       delta
BenchmarkBinaryTree17              1974000000      2012000000      +1.93%
BenchmarkFannkuch11                18673000000     18965000000     +1.56%
BenchmarkFmtFprintfEmpty           5470            5650            +3.29%
BenchmarkFmtFprintfString          14200           14490           +2.04%
BenchmarkFmtFprintfInt             13150           12840           -2.36%
BenchmarkFmtFprintfIntInt          21900           22120           +1.00%
BenchmarkFmtFprintfPrefixedInt     16470           16770           +1.82%
BenchmarkFmtFprintfFloat           43000           44266           +2.94%
BenchmarkFmtManyArgs               84700           86150           +1.71%
BenchmarkGobDecode                 728500000       706500000       -3.02%
BenchmarkGobEncode                 777500000       773500000       -0.51%
BenchmarkGzip                      16180000000     15459000000     -4.46%
BenchmarkGunzip                    4273000000      3927000000      -8.10%
BenchmarkJSONEncode                1960000000      1901000000      -3.01%
BenchmarkJSONDecode                3015000000      3150000000      +4.48%
BenchmarkMandelbrot200             3950000         3950000         +0.00%
BenchmarkGoParse                   109000000       118800000       +8.99%
BenchmarkRegexpMatchEasy0_32       5150            4476            -13.09%
BenchmarkRegexpMatchEasy0_1K       19350           17690           -8.58%
BenchmarkRegexpMatchEasy1_32       4570            3770            -17.51%
BenchmarkRegexpMatchEasy1_1K       30760           27960           -9.10%
BenchmarkRegexpMatchMedium_32      9510            7760            -18.40%
BenchmarkRegexpMatchMedium_1K      2270000         1752000         -22.82%
BenchmarkRegexpMatchHard_32        146500          130700          -10.78%
BenchmarkRegexpMatchHard_1K        4260000         3740000         -12.21%
BenchmarkRevcomp                   14344000000     12848000000     -10.43%
BenchmarkTemplate                  4285000000      4271000000      -0.33%
BenchmarkTimeParse                 15000           15450           +3.00%
BenchmarkTimeFormat                49433           50000           +1.15%

benchmark                         old MB/s     new MB/s     speedup
BenchmarkGobDecode                1.05         1.09         1.04x
BenchmarkGobEncode                0.99         0.99         1.00x
BenchmarkGzip                     1.20         1.26         1.05x
BenchmarkGunzip                   4.54         4.94         1.09x
BenchmarkJSONEncode               0.99         1.02         1.03x
BenchmarkJSONDecode               0.64         0.62         0.97x
BenchmarkGoParse                  0.53         0.49         0.92x
BenchmarkRegexpMatchEasy0_32      6.21         7.15         1.15x
BenchmarkRegexpMatchEasy0_1K      52.92        57.89        1.09x
BenchmarkRegexpMatchEasy1_32      7.00         8.49         1.21x
BenchmarkRegexpMatchEasy1_1K      33.29        36.62        1.10x
BenchmarkRegexpMatchMedium_32     0.11         0.13         1.18x
BenchmarkRegexpMatchMedium_1K     0.45         0.58         1.29x
BenchmarkRegexpMatchHard_32       0.22         0.24         1.09x
BenchmarkRegexpMatchHard_1K       0.24         0.27         1.13x
BenchmarkRevcomp                  17.72        19.78        1.12x
BenchmarkTemplate                 0.45         0.45         1.00x

I'm seeing 10-20% speedups in the regexp matching tests.

Go parsing bench is 9% slower.

The rest are +/-5%.

@neelance
Member

I am surprised that the JS engine does not eliminate the unnecessary right shift, it should be very easy to detect. I am also not sure if your change can replace fixNumber in all cases. I have to investigate it further.

@jbowes
Contributor
jbowes commented Feb 12, 2017

I wasn't expecting this change to have any performance impact; I was just trying to remove some generated code that didn't seem strictly necessary. It was really just something I saw while investigating #584

Feel free to pass on this one.

@shurcooL
Member

I wasn't expecting this change to have any performance impact; I was just trying to remove some generated code that didn't seem strictly necessary.

Ah, thanks for clarifying that. It's helpful to know what the goal is.

@neelance
Member

Yeah, I'd like to pass on this one. I'm not 100% sure that it is correct in all circumstances and I don't have the time to investigate much more. Thanks for the PR anyways. :-)

@neelance neelance closed this Feb 16, 2017
@neelance
Member

@shurcooL Can you really confirm a positive performance impact? Did all tests pass for you? (unfortunately CI skipped this PR)

@shurcooL
Member

What do you mean by confirm a positive performance impact?

I can confirm the comment I made is an accurate representation of my test run. Did you want me to run the benchmarks an extra time to confirm?

I hadn't run the tests locally yet.

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