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.

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

cmd/compile: allow inlining of open-coded defer calls #38471

Open
prattmic opened this issue Apr 15, 2020 · 2 comments
Open

cmd/compile: allow inlining of open-coded defer calls #38471

prattmic opened this issue Apr 15, 2020 · 2 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone
Unplanned

Comments

@prattmic
Copy link
Member

Inlining happens before SSA conversion, while open-coded defers happen after, thus the open-coded deferred call can never be inlined. It would be nice to fix this.

@danscales:

It definitely is not trivial, since inlining happens before SSA conversion, whereas there are a bunch of reasons why at least some of the open-coded defer work have to be done during SSA conversion. Among other things, we need to force storing defer args to stack slots and keeping those stacks slots live even though they sometimes appear to be dead (but are actually needed to run the open-coded defers when there is a panic).

@randall77 @aclements
@prattmic prattmic added this to the Unplanned milestone Apr 15, 2020
@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 16, 2020
@prattmic prattmic mentioned this issue Oct 28, 2020
Closed
@prattmic
Copy link
Member Author

For reference, the difference I see for mutex unlock looks like:

name          time/op
Bare-12       10.2ns ± 1%
OpenDefer-12  12.0ns ± 2%
HeapDefer-12  36.5ns ± 2%

https://play.golang.org/p/JDxJC_9hUgO

@twmb twmb mentioned this issue Nov 12, 2021
Closed
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/424920 mentions this issue: go/token: make mutex locking in unpack cheaper

gopherbot pushed a commit that referenced this issue Aug 22, 2022
@mvdan
go/token: make mutex locking in unpack cheaper
bdecfcb 
I was profiling the cpu usage of go/printer's only benchmark,
and found that token.File.Unpack was one of the top offenders.

It was mainly the deferred unlock that took a big chunk of time,
and to my surprise, reoving the use of defer helped significantly:

	name      old time/op    new time/op    delta
	Print-16    5.61ms ± 2%    5.38ms ± 1%  -4.04%  (p=0.000 n=10+8)

	name      old speed      new speed      delta
	Print-16  9.27MB/s ± 2%  9.64MB/s ± 1%  +4.03%  (p=0.000 n=9+8)

	name      old alloc/op   new alloc/op   delta
	Print-16     332kB ± 0%     332kB ± 0%    ~     (p=0.363 n=10+10)

	name      old allocs/op  new allocs/op  delta
	Print-16     3.45k ± 0%     3.45k ± 0%    ~     (all equal)

It seems like #38471 is to blame, as the defer prevents Unlock from
being inlined. Add a TODO as a reminder to come back here once the
compiler issue is fixed.

Change-Id: I5a1c6d36a8e8357435a305a1bc0970ee0358b08a
Reviewed-on: https://go-review.googlesource.com/c/go/+/424920
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
4 participants
@bradfitz @andybons @prattmic @gopherbot