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

Use concrete types for path steps #129

Merged
merged 1 commit into from Mar 12, 2019

Conversation

Projects
None yet
2 participants
@dsnet
Copy link
Member

dsnet commented Mar 1, 2019

Rather than representing each path step as an interface,
use concrete types instead. This provides some performance benefits as
it reduces the amount of virtual function calls and also provides the
ability for the compiler to inline method calls.

This is technically a breaking change, but since each of the path step
interfaces were explicitly implemented in such a way that they couldn't
be implemented directly (due to the presence of an unexported method),
the only way someone could have been depending on these as interfaces is
if they embedded the interface into another interface. Static analysis of
all code at Google and publicly available on GitHub shows that this is
not a problem.

The performance benefits of this change is significant:

benchmark                               old ns/op     new ns/op     delta
BenchmarkBytes/64KiB/EqualFilter0-4     80551394      46592605      -42.16%
BenchmarkBytes/64KiB/EqualFilter1-4     102922132     69974509      -32.01%
BenchmarkBytes/64KiB/EqualFilter2-4     159009935     94474812      -40.59%
BenchmarkBytes/64KiB/EqualFilter3-4     181231264     124601102     -31.25%
BenchmarkBytes/64KiB/EqualFilter4-4     189775228     148864070     -21.56%
BenchmarkBytes/64KiB/EqualFilter5-4     285065469     175198907     -38.54%

benchmark                               old MB/s     new MB/s     speedup
BenchmarkBytes/64KiB/EqualFilter0-4     1.63         2.81         1.72x
BenchmarkBytes/64KiB/EqualFilter1-4     1.27         1.87         1.47x
BenchmarkBytes/64KiB/EqualFilter2-4     0.82         1.39         1.70x
BenchmarkBytes/64KiB/EqualFilter3-4     0.72         1.05         1.46x
BenchmarkBytes/64KiB/EqualFilter4-4     0.69         0.88         1.28x
BenchmarkBytes/64KiB/EqualFilter5-4     0.46         0.75         1.63x

benchmark                               old allocs     new allocs     delta
BenchmarkBytes/64KiB/EqualFilter0-4     133            134            +0.75%
BenchmarkBytes/64KiB/EqualFilter1-4     134            134            +0.00%
BenchmarkBytes/64KiB/EqualFilter2-4     135            135            +0.00%
BenchmarkBytes/64KiB/EqualFilter3-4     135            135            +0.00%
BenchmarkBytes/64KiB/EqualFilter4-4     136            136            +0.00%
BenchmarkBytes/64KiB/EqualFilter5-4     136            136            +0.00%

benchmark                               old bytes     new bytes     delta
BenchmarkBytes/64KiB/EqualFilter0-4     6632417       6632523       +0.00%
BenchmarkBytes/64KiB/EqualFilter1-4     6632416       6632464       +0.00%
BenchmarkBytes/64KiB/EqualFilter2-4     6632464       6632507       +0.00%
BenchmarkBytes/64KiB/EqualFilter3-4     6632502       6632483       -0.00%
BenchmarkBytes/64KiB/EqualFilter4-4     6632652       6632668       +0.00%
BenchmarkBytes/64KiB/EqualFilter5-4     6632604       6632659       +0.00%

@neild neild self-requested a review Mar 1, 2019

@neild

neild approved these changes Mar 1, 2019

Use concrete types for path steps
Rather than representing each path step as an interface,
use concrete types instead. This provides some performance benefits as
it reduces the amount of virtual function calls and also provides the
ability for the compiler to inline method calls.

This is technically a breaking change, but since each of the path step
interfaces were explicitly implemented in such a way that they couldn't
be implemented directly (due to the presence of an unexported method),
the only way someone could have been depending on these as interfaces is
if they embedded the interface into another interface. Static analysis of
all code at Google and publicly available on GitHub shows that this is
not a problem.

The performance benefits of this change is significant:

benchmark                               old ns/op     new ns/op     delta
BenchmarkBytes/64KiB/EqualFilter0-4     80551394      46592605      -42.16%
BenchmarkBytes/64KiB/EqualFilter1-4     102922132     69974509      -32.01%
BenchmarkBytes/64KiB/EqualFilter2-4     159009935     94474812      -40.59%
BenchmarkBytes/64KiB/EqualFilter3-4     181231264     124601102     -31.25%
BenchmarkBytes/64KiB/EqualFilter4-4     189775228     148864070     -21.56%
BenchmarkBytes/64KiB/EqualFilter5-4     285065469     175198907     -38.54%

benchmark                               old MB/s     new MB/s     speedup
BenchmarkBytes/64KiB/EqualFilter0-4     1.63         2.81         1.72x
BenchmarkBytes/64KiB/EqualFilter1-4     1.27         1.87         1.47x
BenchmarkBytes/64KiB/EqualFilter2-4     0.82         1.39         1.70x
BenchmarkBytes/64KiB/EqualFilter3-4     0.72         1.05         1.46x
BenchmarkBytes/64KiB/EqualFilter4-4     0.69         0.88         1.28x
BenchmarkBytes/64KiB/EqualFilter5-4     0.46         0.75         1.63x

benchmark                               old allocs     new allocs     delta
BenchmarkBytes/64KiB/EqualFilter0-4     133            134            +0.75%
BenchmarkBytes/64KiB/EqualFilter1-4     134            134            +0.00%
BenchmarkBytes/64KiB/EqualFilter2-4     135            135            +0.00%
BenchmarkBytes/64KiB/EqualFilter3-4     135            135            +0.00%
BenchmarkBytes/64KiB/EqualFilter4-4     136            136            +0.00%
BenchmarkBytes/64KiB/EqualFilter5-4     136            136            +0.00%

benchmark                               old bytes     new bytes     delta
BenchmarkBytes/64KiB/EqualFilter0-4     6632417       6632523       +0.00%
BenchmarkBytes/64KiB/EqualFilter1-4     6632416       6632464       +0.00%
BenchmarkBytes/64KiB/EqualFilter2-4     6632464       6632507       +0.00%
BenchmarkBytes/64KiB/EqualFilter3-4     6632502       6632483       -0.00%
BenchmarkBytes/64KiB/EqualFilter4-4     6632652       6632668       +0.00%
BenchmarkBytes/64KiB/EqualFilter5-4     6632604       6632659       +0.00%

@dsnet dsnet force-pushed the concrete-steps branch from f9489fa to e2cb7d7 Mar 12, 2019

@dsnet dsnet merged commit 49488b4 into master Mar 12, 2019

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@dsnet dsnet deleted the concrete-steps branch Mar 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.