x/mobile/geom: concurrent modification to Width, Height global vars #10461
Comments
|
I would imagine that they are not intended to be accessed except in the UI goroutine (i.e. |
|
There is a goroutine, locked to an OS thread, that is both responsible for updating geom.{Width, Height} and calling app.Draw. So there is no data race at the moment as long as width and height are only used from the Draw function (which is also the only safe place to use GL). This however, is not a particularly goroutine-friendly design, and I'm open to alternatives. |
|
My current thinking on this is that these values should be folded into the With a package function, But I'm not going to think about this again until after the Go 1.5 freeze. Then I'll prototype it in some apps and if it works, send out a CL. I believe there's some other Issue opened somewhere about app configuration updates, this would cover that too. |
|
I like the idea of passing the current state to a callback. As an improvement, making the state available on each callback would be more user-friendly. Start func(c Config)
Stop func(c Config)
Draw func(c Config)
Touch func(c Config, e event.Touch)#10442 lists a number of other configuration related properties. We might provide a few of them as a part of Config if querying them are not costly. |
|
CL https://golang.org/cl/9708 mentions this issue. |
Config provides a way to concurrently access Width and Height. Register provides a way for packages to run code on app state change without plumbing changes all the way to the process main function. This is motivated by gl/glutil.Image which needs to rebuild its textures on start/stop and can be deeply nested. (See golang.org/cl/9707 for the followup.) Tested manually on android and darwin/amd64. Doing this kind makes it clear any CL modifying this code needs a lot of manual testing right now, so some kind of trybot support is something I'm going to prioritise. Fixes golang/go#10686 Fixes golang/go#10461 Fixes golang/go#10442 Fixes golang/go#10226 Updates golang/go#10327 Change-Id: I2882ebf3995b6ed857cda823e94fbb17c54b43a8 Reviewed-on: https://go-review.googlesource.com/9708 Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Config provides a way to concurrently access Width and Height. Register provides a way for packages to run code on app state change without plumbing changes all the way to the process main function. This is motivated by gl/glutil.Image which needs to rebuild its textures on start/stop and can be deeply nested. (See golang.org/cl/9707 for the followup.) Tested manually on android and darwin/amd64. Doing this kind makes it clear any CL modifying this code needs a lot of manual testing right now, so some kind of trybot support is something I'm going to prioritise. Fixes golang/go#10686 Fixes golang/go#10461 Fixes golang/go#10442 Fixes golang/go#10226 Updates golang/go#10327 Change-Id: I2882ebf3995b6ed857cda823e94fbb17c54b43a8 Reviewed-on: https://go-review.googlesource.com/9708 Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
geom.Width and geom.Height look like they're meant to be initialized once and not mutated afterwards, since they're plain global variables without any locking.
However, x/mobile/app and in particular loop_android.go updates these global vars when e.g. the phone rotates.
There is no clear line between the UI goroutine and the others. Should we protect each value or should we restrict them being accessible from the UI goroutine? This is a design question and requires discussion.
The text was updated successfully, but these errors were encountered: