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

server: reimplement growStack hack in assembly #37967

Merged
merged 1 commit into from Jun 3, 2019

Conversation

Projects
None yet
4 participants
@petermattis
Copy link
Contributor

commented Jun 1, 2019

The go1.12 compiler breaks the Go version of the growStack hack (it
optimizes the 16kb stack variable out of existence). Reimplement
growStack in assembly which will be same from the machinations of the Go
compiler.

Release note (performance improvement): Fix a significant performance
regression when building CockroachDB with go1.12.

server: reimplement growStack hack in assembly
The go1.12 compiler breaks the Go version of the growStack hack (it
optimizes the 16kb stack variable out of existence). Reimplement
growStack in assembly which will be same from the machinations of the Go
compiler.

Release note (performance improvement): Fix a significant performance
regression when building CockroachDB with go1.12.

@petermattis petermattis requested a review from cockroachdb/core-prs as a code owner Jun 1, 2019

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

commented Jun 1, 2019

This change is Reviewable

@petermattis

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2019

See #37966

@petermattis petermattis requested review from nvanbenschoten and bdarnell Jun 3, 2019

@nvanbenschoten
Copy link
Member

left a comment

LGTM. Nice find. Any idea what change in the Go compiler caused the old hack to be optimized away?

The symptoms you saw in #37966 (comment) look awfully similar to what @jeffrey-xiao and I are seeing in #37706. It's possible that we're seeing a very similar issue where the difference in proto padding is causing stack growth underneath the HLC clock mutex (instead of somewhere else) and destroying performance. Once this is merged, we should dig into whether we can have any effect on the perf drop by playing around with this new Grow function. Go 1.12 should also help us determine whether we can attribute slowdown of the clock to stack growth.

@petermattis

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

bors r=nvanbenschoten

craig bot pushed a commit that referenced this pull request Jun 3, 2019

Merge #37967
37967: server: reimplement growStack hack in assembly r=nvanbenschoten a=petermattis

The go1.12 compiler breaks the Go version of the growStack hack (it
optimizes the 16kb stack variable out of existence). Reimplement
growStack in assembly which will be same from the machinations of the Go
compiler.

Release note (performance improvement): Fix a significant performance
regression when building CockroachDB with go1.12.

Co-authored-by: Peter Mattis <petermattis@gmail.com>
@craig

This comment has been minimized.

Copy link

commented Jun 3, 2019

Build succeeded

@craig craig bot merged commit a2d0d6b into cockroachdb:master Jun 3, 2019

5 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
security/snyk - Gopkg.lock (tim) No manifest changes detected
security/snyk - pkg/ui/package.json (tim) No manifest changes detected
@bdarnell

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

No GNU_STACK magic? Did my test not run or does this somehow avoid setting the exec-stack flag?

@petermattis

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

@bdarnell

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

I don't think being empty matters. Or at least it's not sufficient; the empty file in the sql/exec package doesn't trigger it. There's something we don't understand going on here (my best guess is that it has something to do with whether or not cgo is in use in the package).

@petermattis petermattis deleted the petermattis:pmattis/growstack branch Jun 4, 2019

@petermattis

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

@bdarnell I ran scanelf -qe on a cockroach binary built with this PR and it didn't complain.

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.