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

cmd/compile: go:linkname prevents inlining #20019

Open
dvyukov opened this Issue Apr 18, 2017 · 13 comments

Comments

Projects
None yet
6 participants
@dvyukov
Member

dvyukov commented Apr 18, 2017

go version devel +33c3477039 Tue Apr 18 03:56:16 2017 +0000 linux/amd64

sync_runtime_procPin is marked with //go:linkname sync_runtime_procPin sync.runtime_procPin. Runtime marks it as inlinable (which is good). However, sync fails to inline it with cannot inline runtime_canSpin: no function body.
There are more functions exposed from runtime to sync, sync/atomic, reflect, time, os.
Can we inline them?

@dvyukov

This comment has been minimized.

Member

dvyukov commented Apr 18, 2017

@valyala

This comment has been minimized.

Contributor

valyala commented Apr 18, 2017

cc'ing @josharian

@josharian

This comment has been minimized.

Contributor

josharian commented Apr 18, 2017

At first glance, looks like an import/export issue--the function body is not associated correctly (or perhaps not exported at all) when linkname is present.

cc @mdempsky @griesemer

@josharian josharian added this to the Go1.9Maybe milestone Apr 18, 2017

@dvyukov

This comment has been minimized.

Member

dvyukov commented Apr 18, 2017

It's possible to quickly estimate the benefits -- make procPin/Unpin public in runtime and directly call from sync.Pool. @valyala care to prototype?

@cherrymui

This comment has been minimized.

Contributor

cherrymui commented Apr 18, 2017

@josharian If I understand correctly, linkname is resolved at link time (as the name suggests). The runtime package doesn't export those names.

@valyala

This comment has been minimized.

Contributor

valyala commented Apr 18, 2017

It's possible to quickly estimate the benefits -- make procPin/Unpin public in runtime and directly call from sync.Pool. @valyala care to prototype?

@dvyukov , sure:

GOMAXPROCS=4:

name            old time/op  new time/op  delta
Pool-4          10.5ns ± 9%   8.8ns ± 9%  -15.59%  (p=0.000 n=10+10)
PoolOverflow-4  2.10µs ± 3%  1.95µs ± 2%   -7.29%  (p=0.000 n=10+10)

GOMAXPROCS=1:

name          old time/op  new time/op  delta
Pool          20.3ns ± 2%  16.9ns ± 3%  -17.06%  (p=0.000 n=10+10)
PoolOverflow  4.96µs ± 2%  4.75µs ± 1%   -4.16%  (p=0.000 n=10+9)
@josharian

This comment has been minimized.

Contributor

josharian commented Apr 18, 2017

@cherrymui but I believe we do have inlining information in the export data for non-exported function, specifically for (multi-level) inlining.

@cherrymui

This comment has been minimized.

Contributor

cherrymui commented Apr 18, 2017

@josharian you're right, thanks!
But the package that have functions implemented by another package may not import that package. sync/atomic doesn't import runtime, and the compiler doesn't even read runtime.a when compiling sync/atomic. For this to work, we'll need to add import _ "runtime" in sync/atomic?

@josharian

This comment has been minimized.

Contributor

josharian commented Apr 18, 2017

@cherrymui true. And that doesn't seem too much to ask, if you're already using go:linkname. :) Although this also reproduces with package sync (in pool.go), which does already import package runtime.

@mdempsky

This comment has been minimized.

Member

mdempsky commented Apr 18, 2017

I think the issue is just in bimport.go and trivially fixable. @dvyukov do you have an easy standalone test case for this?

@dvyukov

This comment has been minimized.

Member

dvyukov commented Apr 18, 2017

@mdempsky no, I don't. I was looking at sync_runtime_procPin.

@mdempsky

This comment has been minimized.

Member

mdempsky commented Apr 18, 2017

Thanks. I can repro the issue, but realized it's more involved than simply tweaking bimport.go.

Currently, we associate function inlining bodies only at the individual Node level. To fix this, we'll need to cross-reference them via LSyms.

@josharian

This comment has been minimized.

Contributor

josharian commented Apr 18, 2017

Not sure that's worth it at the moment.

@bradfitz bradfitz modified the milestones: Go1.9Maybe, Go1.10 Jul 20, 2017

@bradfitz bradfitz modified the milestones: Go1.10, Go1.11 Nov 28, 2017

@bradfitz bradfitz modified the milestones: Go1.11, Unplanned May 18, 2018

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