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

syscall/js: remove Wrapper type to avoid extreme allocations and improve performance #44006

Closed
finnbear opened this issue Jan 30, 2021 · 19 comments

Comments

@finnbear
Copy link

finnbear commented Jan 30, 2021

Introduction

It is truly remarkable what can be accomplished by compiling Go to WebAssembly (wasm), but virtually all such applications are being held back by the combination of two factors:

  1. Every function call to JS that passes arguments makes multiple, unavoidable Go heap allocations
  2. The Go garbage collector then blocks the main, and only, thread for a large fraction of a second

As the title of this proposal suggests, the performance problems can be solved if the js.Wrapper type is removed. Removal of existing features from the standard library should not be taken lightly, but this particular removal is both allowable (syscall/js is exempt from Go's compatibility promise) and necessary given the data.

Why Wrapper matters

At the core of every call to JS, syscall/js uses its ValueOf function to convert Go arguments to their JS equivalents. This conversion is implemented as a switch statement:

func ValueOf(x interface{}) Value {
	switch x := x.(type) { // some cases omitted
	case Value:
		return x
	case Wrapper:
		return x.JSValue() // the problem
	case nil:
		return valueNull
	case bool:
		if x {
			return valueTrue
		} else {
			return valueFalse
		}
	case int:
		return floatValue(float64(x))
	case unsafe.Pointer:
		return floatValue(float64(uintptr(x)))
	case float64:
		return floatValue(x)
	case string:
		return makeValue(stringVal(x))
	default:
		panic("ValueOf: invalid value")
	}
}

One of the cases is the Wrapper interface. This was intended to allow any type implementing the JSValue method to be easily passed as an argument to a JS method. However, as JSValue may consist of an arbitrary pointer receiver, the compiler must assume the worst:

type BadWrapper struct {
    Value js.Value
}

var escapeRoute *BadWrapper

// Implements js.Wrapper
func (this *BadWrapper) JSValue() js.Value {
    escapeRoute = this // escape to heap
    return this.Value
}

This means that the argument to ValueOf, which includes any argument passed to any JS function, is guaranteed to escape to the heap.

Data

Data is reported as number of heap allocations per unit of work.

Optimizations (cumulative) Frame of game (best case) Frame of game (worst case) Typical canvas/WebGL call []byte copy to JS string manipulation Garbage collector effect on gameplay
Control (no optimizations) 858 40000 3 1 3 Frequent stutters
//go:noescape 829 untested 3 0 3 Frequent stutters
makeArgs slices on stack (See #39740) 383 untested 1 0 3 Less-frequent stutters
Unexport Wrapper.JSValue() 383 untested 1 0 3 No change
Remove Wrapper type 62* 300* 0 0 2 Not bad, especially after other allocations are reduced

*on the order of the number of allocations that happen unrelated to syscall/js

Data collection method

  1. Debug output from my online game, implemented in Go
  2. Micro-benchmarks: main.go
  3. Entire patch to syscall/js/js.go: js.go.patch

Context

Preliminary discussion of this proposal has has already taken place in #39740. The consensus there was that removing Wrapper is the only way to sustainably call JS functions in a performance-critical setting. Given that syscall/js is a low level package, it make sense to prioritize performance over the ease of use afforded by Wrapper.

More context on my actual use case

  1. My game's client is written in Go, compiled to WebAssembly, uses WebGL 2.0 APIs, and is intended to run at 60 frames per second.
  2. The WebGL API requires many function calls, each mainly taking integer and floating point parameters.
  3. I have observed to Go garbage collector running every 3-5 seconds under normal circumstances.
  4. I have observed the Go garbage collector running over 10 times in a single frame in a worst-case-scenario
  5. I have observed the Go garbage collector taking 300+ milliseconds to run
@hajimehoshi
Copy link
Member

CC @neelance @agnivade

@neelance
Copy link
Member

I support this proposal. The Wrapper type was only added for convenience and performance trumps convenience for such a low-level package as syscall/js.

The author of the Wrapper type also supports the removal: #39740 (comment)

@rsc Please consider this proposal for the proposal process.

BenLubar added a commit to BenLubar-PR/golang-go that referenced this issue Jun 3, 2021
The existence of the Wrapper interface causes unavoidable heap
allocations in syscall/js.ValueOf. See the linked issue for a full
explanation.

Fixes golang#44006.
@awfulcooking
Copy link

It is truly remarkable what can be accomplished by compiling Go to WebAssembly

I have a program which animates an LED matrix, and transfers frames @ 60fps, to a JS IDE for rendering.

Receiving binary frames over a WebSocket, the IDE starts to stutter around 9000 - 10,000 pixels.

Receiving binary frames from embedded Go to JS, the server both rendering the animation and copying the bytes to a JS buffer within the context of a requestAnimationFrame, the IDE can handle 10,000 pixels like butter.. without breaking a sweat!

Reading this proposal, and hearing of performance improvements in 1.17, gives hope that even better performance is possible!

Exciting times for the web platform and Go both.

@neelance
Copy link
Member

neelance commented Sep 9, 2021

Any chance to get this into the proposal process this cycle? 🙂

@rsc
Copy link
Contributor

rsc commented Sep 15, 2021

Do we have any numbers about how often Wrapper is used today? Did I miss them above?

@rsc
Copy link
Contributor

rsc commented Sep 15, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals (old) Sep 15, 2021
@finnbear
Copy link
Author

finnbear commented Sep 15, 2021

Do we have any numbers about how often Wrapper is used today? Did I miss them above?

You didn't miss anything; the original proposal didn't make an effort to enumerate usage of js.Wrapper, but it probably should have.

Anyway, searching https://github.com/search?q=%22js.Wrapper%22+language%3AGo&type=Code returns 375 code results.

  • For comparison, the close alternative of js.Value returns 2,790 results and syscall/js returns 16,253 results. Based on these numbers alone, approximately 2-13% of the open-source Go syscall/js ecosystem is affected. I attribute this low percentage to the fact that it is easy to develop applications without js.Wrapper (as an interface, it is mainly a convenience for library developers).
  • The vast majority of the js.Wrapper results are false positives (i.e. from projects that use JS and "wrappers," but not syscall/js). Many of the code results are in comments, so the actual usage of the js.Wrapper type is less. I made no attempt to check what fraction of the js.Value results are false positives, since there are too many.

Here is a (potentially incomplete) list of projects that would (at least temporarily) break (they reference the js.Wrapper type). The list starts with what seemed to be "the big ones" and then proceeds in no particular order. Of these projects, some rely on js.Wrapper simply to call .JSValue() themselves, instead of actually passing to Call, New, or Invoke (which could be adapted to passing around js.Value instead).

It would be interesting to get the perspectives of the maintainers of these projects.

Finally, since it's been 8 months since this proposal began, I have more context on my particular use cases: Development of the Go version of my online game has stopped, and development on a Rust port has started (but stalled). I also started developing a new online Game based on Rust wasm. Rust seems to be a better fit for performance critical wasm. While I would not personally benefit from this proposal any more, I think it would help close the Go<->Rust wasm gap by eliminating the major performance cliff.

@hajimehoshi
Copy link
Member

hajimehoshi commented Sep 16, 2021

Anyway, searching https://github.com/search?q=%22js.Wrapper%22+language%3AGo&type=Code returns 375 code results.

https://github.com/search?q=%22syscall%2Fjs%22+%22js.Wrapper%22+language%3AGo&type=Code I searched this with syscall/js and the number of results was 55.

As this is an interface, we should find the code that defines a method JSValue() js.Value, as you have already mentioned.

@rsc
Copy link
Contributor

rsc commented Sep 22, 2021

It sounds like we need more data about the potential impact here.

@rsc
Copy link
Contributor

rsc commented Oct 6, 2021

I would still feel better with more data, but because this is in syscall and in wasm, which is not heavily used, and because literally no one has objected to doing this, it seems okay to do it and see if anyone comes forward.

@rsc
Copy link
Contributor

rsc commented Oct 6, 2021

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Oct 6, 2021
@cherrymui
Copy link
Member

This is not really about removing Wrapper type, which I have no objection. But I don't really understand the escaping part. What exactly escapes, and can be avoided if Wrapper is removed? And if a value is passed to JavaScript how do we know it is safe to not escape? I.e. be sure that JavaScript doesn't hold the value or use it in some asynchronous way? Thanks.

@finnbear
Copy link
Author

finnbear commented Oct 7, 2021

What exactly escapes

From Go's perspective, the parameter x to ValueOf escapes because its data potion can escape. All interfaces in Go operate via indirection. They have one pointer to type information and one pointer to data. The interface itself may be copied by value, but that data must live on the stack or heap. If there is any chance the interface or it's data could escape off the stack, the Go compiler must ensure that the interface's data lives on the heap (by allocating the data that goes into the interface on the heap, e.g. when an argument is passed to a function that takes an interface parameter).

and can be avoided if Wrapper is removed?

As outlined in my original proposal, the case Wrapper is the deciding factor for whether the interface's data can escape. Because Wrapper is, in itself, an interface, its dynamically-dispatched JSValue receiver can run arbitrary code using the interface's data, which may be a pointer (including storing that pointer globally or on the heap, but crucially, off the stack; see BadWrapper). For all other cases in the switch statement, the conversion to JS is statically dispatched so the Go compiler knows that nothing escapes.

And if a value is passed to JavaScript how do we know it is safe to not escape? I.e. be sure that JavaScript doesn't hold the value or use it in some asynchronous way?

It is unsafe for JS to hold pointers into Go's memory, because if Go allocates memory, it may need to expand its WebAssembly linear memory array buffer. At that point, JS would hold stale pointers. I assume it was for this reason that whoever designed syscall/js made it copy all values to JS. For example, when you pass a string to a JS function, the string is copied to JS, and becomes a new JS allocation that is independent from the Go string. Anyway, at least in my understanding, this design decision means that Go values will not escape to the Go or JS heap due to being passed to JS. But they could easily escape with a BadWrapper.

Disclaimer: This is all just my understanding, gathered by optimizing Go and syscall/js code. I'm not a Go compiler developer. Feel free to correct me if I said anything wrong or misleading!

@cherrymui
Copy link
Member

Thanks. Yeah, I think it makes sense and is safe for ValueOf not escape the interface data at least in some cases (e.g. numbers).

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Oct 13, 2021
@rsc
Copy link
Contributor

rsc commented Oct 13, 2021

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: syscall/js: remove Wrapper type to avoid extreme allocations and improve performance syscall/js: remove Wrapper type to avoid extreme allocations and improve performance Oct 13, 2021
@rsc rsc modified the milestones: Proposal, Backlog Oct 13, 2021
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/356430 mentions this issue: syscall/js: remove Wrapper interface

@finnbear
Copy link
Author

finnbear commented Oct 18, 2021

Change https://golang.org/cl/356430 mentions this issue: syscall/js: remove Wrapper interface

Great! Don't forget to apply the other optimizations (e.g. noescape; none of which are breaking changes; see my patch) so the performance gain will apply 😉

@neelance
Copy link
Member

@finnbear Let's continue our discussion in #39740.

@dmitshur dmitshur modified the milestones: Backlog, Go1.18 Oct 20, 2021
@dmitshur
Copy link
Contributor

Updated the milestone to Go 1.18 since that's the version this proposal was implemented.

This change is significant enough to warrant being mentioned in the 1.18 release notes.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

8 participants