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

Jump to bottom

cmd/compile: investigate wasm sync inlining #30605

Open
bradfitz opened this issue Mar 5, 2019 · 2 comments
Open

cmd/compile: investigate wasm sync inlining #30605

bradfitz opened this issue Mar 5, 2019 · 2 comments
Labels
arch-wasm WebAssembly issues 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

@bradfitz
Copy link
Contributor

bradfitz commented Mar 5, 2019

https://go-review.googlesource.com/c/go/+/148958/ made sync.Mutex.Unlock inlinable.

It worked for Wasm, but then CL 164461 and CL 153797 landed and the resulting merge no longer passed the inlining tests.

As a quick fix, https://go-review.googlesource.com/c/go/+/165339 ignored wasm in the inlining tests.

Investigate & re-enable.

/cc @ALTree @neelance @aclements @CAFxX
@bradfitz bradfitz added Performance arch-wasm WebAssembly issues labels Mar 5, 2019
@gopherbot
Copy link

Change https://golang.org/cl/165339 mentions this issue: test: skip mutex Unlock inlining tests on a few builders

gopherbot pushed a commit that referenced this issue Mar 5, 2019
@ALTree @bradfitz
test: skip mutex Unlock inlining tests on a few builders
2f8d242 
Fix builder breakage from CL 148958.

This is an inlining test that should be skipped on -N -l.

The inlining also doesn't happen on arm and wasm, so skip the test
there too.

Fixes the noopt builder, the linux-arm builder, and the wasm builder.

Updates #30605

Change-Id: I06b90d595be7185df61db039dd225dc90d6f678f
Reviewed-on: https://go-review.googlesource.com/c/go/+/165339
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@neelance
Copy link
Member

The test now explains:

// FIXME: This test is disabled on architectures where atomic operations
// are function calls rather than intrinsics, since this prevents inlining
// of the sync fast paths. This test should be re-enabled once the problem
// is solved.

This applies to wasm since it has no atomic operations yet. The test needs to be reenabled after the threads proposal landed.

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 12, 2019
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
@seankhliao seankhliao added this to the Unplanned milestone Aug 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
arch-wasm WebAssembly issues 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
Go Compiler / Runtime
Status: Triage Backlog
Milestone
Unplanned
Development

No branches or pull requests
5 participants
@bradfitz @neelance @bcmills @gopherbot @seankhliao