Skip to content

reflect: cache IsVariadic calls in Call #43475

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

reflect: cache IsVariadic calls in Call #43475

wants to merge 3 commits into from

Conversation

jsign
Copy link
Contributor

@jsign jsign commented Jan 3, 2021 •
edited by mvdan
Loading

These calls are cacheable, so do that to avoid doing extra work.

This opportunity was discovered while taking a look at a CPU profile
while investigating #7818.

I added a BenchmarkCallMethod, which is similar to BechmarkCall but
for a method receiver.

Benchmark results, including the new BenchmarkCallMethod:

name                       old time/op    new time/op    delta
Call-16                      22.0ns ±19%    20.2ns ±17%  -8.08%  (p=0.000 n=40+40)
CallMethod-16                 100ns ± 3%      91ns ± 2%  -9.13%  (p=0.000 n=40+39)
CallArgCopy/size=128-16      15.7ns ± 1%    14.3ns ± 4%  -8.98%  (p=0.000 n=38+37)
CallArgCopy/size=256-16      15.9ns ± 3%    15.0ns ± 5%  -6.12%  (p=0.000 n=39+39)
CallArgCopy/size=1024-16     18.8ns ± 6%    17.1ns ± 6%  -9.03%  (p=0.000 n=38+38)
CallArgCopy/size=4096-16     26.6ns ± 3%    25.2ns ± 4%  -5.19%  (p=0.000 n=39+40)
CallArgCopy/size=65536-16     379ns ± 3%     371ns ± 5%  -2.11%  (p=0.000 n=39+40)

name                       old alloc/op   new alloc/op   delta
Call-16                       0.00B          0.00B         ~     (all equal)
CallMethod-16                 0.00B          0.00B         ~     (all equal)

name                       old allocs/op  new allocs/op  delta
Call-16                        0.00           0.00         ~     (all equal)
CallMethod-16                  0.00           0.00         ~     (all equal)

name                       old speed      new speed      delta
CallArgCopy/size=128-16    8.13GB/s ± 1%  8.92GB/s ± 4%  +9.77%  (p=0.000 n=38+38)
CallArgCopy/size=256-16    16.1GB/s ± 3%  17.1GB/s ± 5%  +6.56%  (p=0.000 n=39+39)
CallArgCopy/size=1024-16   54.6GB/s ± 6%  60.1GB/s ± 5%  +9.93%  (p=0.000 n=38+38)
CallArgCopy/size=4096-16    154GB/s ± 5%   163GB/s ± 4%  +5.63%  (p=0.000 n=40+40)
CallArgCopy/size=65536-16   173GB/s ± 3%   177GB/s ± 5%  +2.18%  (p=0.000 n=39+40)

Updates #7818.

@google-cla google-cla bot added the cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. label Jan 3, 2021
@jsign jsign marked this pull request as ready for review January 3, 2021 15:05
@gopherbot
Copy link
Contributor

This PR (HEAD: 659bc04) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/281252 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Brad Fitzpatrick:

Patch Set 1: Run-TryBot+1 Trust+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/281252.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 1:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=3d4c37e4

Please don’t reply on this GitHub thread. Visit golang.org/cl/281252.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 1: TryBot-Result+1

TryBots are happy.

Please don’t reply on this GitHub thread. Visit golang.org/cl/281252.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Keith Randall:

Patch Set 1: Code-Review+2

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/281252.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Ignacio Hagopian:

Patch Set 1:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/281252.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 0e700c2) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/281252 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Ignacio Hagopian:

Patch Set 2:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/281252.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Daniel Martí:

Patch Set 2: Code-Review+2

(2 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/281252.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Ignacio Hagopian:

Patch Set 2:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/281252.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Daniel Martí:

Patch Set 2:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/281252.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Ignacio Hagopian:

Patch Set 3:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/281252.
After addressing review feedback, remember to publish your drafts!

jsign added 3 commits March 30, 2021 18:13
@jsign @mvdan
reflect: add benchmark for method call
6d8084c 
Signed-off-by: Ignacio Hagopian <[email protected]>
@jsign @mvdan
reflect: cache IsVariadic calls in Call
6e7a431 
These calls are cachable, so do that in order to avoid doing extra work.

This opportunity was discovered while taking a look at a CPU profile while
investigating golang#7818.

I added a BenchmarkCallMethod which is similar to BechmarkCall but for a
method receiver.

Benchmark results, including the new BenchmarkCallMethod:

name                       old time/op    new time/op    delta
Call-16                      22.0ns ±19%    20.2ns ±17%  -8.08%  (p=0.000 n=40+40)
CallMethod-16                 100ns ± 3%      91ns ± 2%  -9.13%  (p=0.000 n=40+39)
CallArgCopy/size=128-16      15.7ns ± 1%    14.3ns ± 4%  -8.98%  (p=0.000 n=38+37)
CallArgCopy/size=256-16      15.9ns ± 3%    15.0ns ± 5%  -6.12%  (p=0.000 n=39+39)
CallArgCopy/size=1024-16     18.8ns ± 6%    17.1ns ± 6%  -9.03%  (p=0.000 n=38+38)
CallArgCopy/size=4096-16     26.6ns ± 3%    25.2ns ± 4%  -5.19%  (p=0.000 n=39+40)
CallArgCopy/size=65536-16     379ns ± 3%     371ns ± 5%  -2.11%  (p=0.000 n=39+40)

name                       old alloc/op   new alloc/op   delta
Call-16                       0.00B          0.00B         ~     (all equal)
CallMethod-16                 0.00B          0.00B         ~     (all equal)

name                       old allocs/op  new allocs/op  delta
Call-16                        0.00           0.00         ~     (all equal)
CallMethod-16                  0.00           0.00         ~     (all equal)

name                       old speed      new speed      delta
CallArgCopy/size=128-16    8.13GB/s ± 1%  8.92GB/s ± 4%  +9.77%  (p=0.000 n=38+38)
CallArgCopy/size=256-16    16.1GB/s ± 3%  17.1GB/s ± 5%  +6.56%  (p=0.000 n=39+39)
CallArgCopy/size=1024-16   54.6GB/s ± 6%  60.1GB/s ± 5%  +9.93%  (p=0.000 n=38+38)
CallArgCopy/size=4096-16    154GB/s ± 5%   163GB/s ± 4%  +5.63%  (p=0.000 n=40+40)
CallArgCopy/size=65536-16   173GB/s ± 3%   177GB/s ± 5%  +2.18%  (p=0.000 n=39+40)

Updates golang#7818.

Signed-off-by: Ignacio Hagopian <[email protected]>
@jsign @mvdan
simplify new benchmark
9bbaa18 
Signed-off-by: Ignacio Hagopian <[email protected]>
@mvdan mvdan force-pushed the jsign/improve7818 branch from 0e700c2 to 9bbaa18 Compare March 30, 2021 17:14
@gopherbot
Copy link
Contributor

This PR (HEAD: 9bbaa18) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/281252 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Daniel Martí:

Patch Set 4: Run-TryBot+1 Code-Review+2 Trust+1

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/281252.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 4:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=d7b84779

Please don’t reply on this GitHub thread. Visit golang.org/cl/281252.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Daniel Martí:

Patch Set 5: Run-TryBot+1 Code-Review+2 Trust+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/281252.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 5:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=2cc4d84a

Please don’t reply on this GitHub thread. Visit golang.org/cl/281252.
After addressing review feedback, remember to publish your drafts!

gopherbot pushed a commit that referenced this pull request Mar 30, 2021
@jsign @mvdan
reflect: cache IsVariadic calls in Call
6cadfe2 
These calls are cacheable, so do that to avoid doing extra work.

This opportunity was discovered while taking a look at a CPU profile
while investigating #7818.

I added a BenchmarkCallMethod, which is similar to BechmarkCall but
for a method receiver.

Benchmark results, including the new BenchmarkCallMethod:

	name                       old time/op    new time/op    delta
	Call-16                      22.0ns ±19%    20.2ns ±17%  -8.08%  (p=0.000 n=40+40)
	CallMethod-16                 100ns ± 3%      91ns ± 2%  -9.13%  (p=0.000 n=40+39)
	CallArgCopy/size=128-16      15.7ns ± 1%    14.3ns ± 4%  -8.98%  (p=0.000 n=38+37)
	CallArgCopy/size=256-16      15.9ns ± 3%    15.0ns ± 5%  -6.12%  (p=0.000 n=39+39)
	CallArgCopy/size=1024-16     18.8ns ± 6%    17.1ns ± 6%  -9.03%  (p=0.000 n=38+38)
	CallArgCopy/size=4096-16     26.6ns ± 3%    25.2ns ± 4%  -5.19%  (p=0.000 n=39+40)
	CallArgCopy/size=65536-16     379ns ± 3%     371ns ± 5%  -2.11%  (p=0.000 n=39+40)

	name                       old alloc/op   new alloc/op   delta
	Call-16                       0.00B          0.00B         ~     (all equal)
	CallMethod-16                 0.00B          0.00B         ~     (all equal)

	name                       old allocs/op  new allocs/op  delta
	Call-16                        0.00           0.00         ~     (all equal)
	CallMethod-16                  0.00           0.00         ~     (all equal)

	name                       old speed      new speed      delta
	CallArgCopy/size=128-16    8.13GB/s ± 1%  8.92GB/s ± 4%  +9.77%  (p=0.000 n=38+38)
	CallArgCopy/size=256-16    16.1GB/s ± 3%  17.1GB/s ± 5%  +6.56%  (p=0.000 n=39+39)
	CallArgCopy/size=1024-16   54.6GB/s ± 6%  60.1GB/s ± 5%  +9.93%  (p=0.000 n=38+38)
	CallArgCopy/size=4096-16    154GB/s ± 5%   163GB/s ± 4%  +5.63%  (p=0.000 n=40+40)
	CallArgCopy/size=65536-16   173GB/s ± 3%   177GB/s ± 5%  +2.18%  (p=0.000 n=39+40)

Updates #7818.

Change-Id: I94f88811ea9faf3dc2543984a13b360b5db66a4b
GitHub-Last-Rev: 9bbaa18
GitHub-Pull-Request: #43475
Reviewed-on: https://go-review.googlesource.com/c/go/+/281252
Reviewed-by: Daniel Martí <[email protected]>
Reviewed-by: Keith Randall <[email protected]>
Trust: Daniel Martí <[email protected]>
Trust: Brad Fitzpatrick <[email protected]>
Run-TryBot: Daniel Martí <[email protected]>
TryBot-Result: Go Bot <[email protected]>
@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 5: TryBot-Result+1

TryBots are happy.

Please don’t reply on this GitHub thread. Visit golang.org/cl/281252.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR is being closed because golang.org/cl/281252 has been merged.

@gopherbot gopherbot closed this Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jsign @gopherbot