Closed Bug 1658419 Opened 4 years ago Closed 2 years ago

Move GamepadPlatformService/GamepadManager to communicate state over shared memory

Categories

(Core :: DOM: Device Interfaces, defect)

Product:
Core
Component:
DOM: Device Interfaces
Type:
defect

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: cmartin, Assigned: cmartin)

References

(Blocks 1 open bug)

Details

Attachments

(4 obsolete files)

The current use of message-passing for this is too much overhead (delays can be caused by threads that are busy, many children processes existing, etc).

We should switch to shared memory and save the message-passing for the things that are more naturally "event-based", like GamepadAdded/GamepadRemoved.

This class (which may be useful outside of just gamepad code) provides an API to share an object across processes in a synchronized manner.

Eventually it will be used to share gamepad data (axes, buttons, etc) immediately with child processes for performance reasons.

PGamepadEventChannelParent will need to send an async message back to the
child with info about the new shared memory region upon creation.

The IPDL channel is not ready during allocation, so we need to defer
registering the actor with GamepadPlatformService until the IPDL
"RecvConstructor" message so the channel will be available.

This is just a straight refactor to prepare for the upcoming change.

Add the scaffolding to setup the shared memory GamepadState between the
GamepadPlatformService and GamepadManager. The next changeset will actually
integrate the new broadcast infrastructure into the gamepad code.

Finally, use the primitives from the previous change to deliver gamepad changes.

If the shared memory shortcut is available, all gamepad changes will be
delivered over it. When the children receive the signal, they will diff their
last-known state against the new state and generate events to update JS.

Pushed by cmartin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/afb09833eddb
Create a class that implements sync'd cross-process shared memory r=handyman
https://hg.mozilla.org/integration/autoland/rev/bbf68f57807b
Change PGamepadEventChannelParent to have a constructor r=handyman
https://hg.mozilla.org/integration/autoland/rev/2b03fe79caef
Add a skeleton GamepadStateBroadcast r=handyman
https://hg.mozilla.org/integration/autoland/rev/0cf10f5443cc
Use shared memory for gamepad state on Windows r=handyman

Backed out for linux bustage on GamepadStateBroadcaster.cpp

backout: https://hg.mozilla.org/integration/autoland/rev/ebae96ff3298dbd37cf617af096d57bd29be2efd

push: https://treeherder.mozilla.org/jobs?repo=autoland&searchStr=linux%2Cx64%2Cdebug%2Cbuild-linux64-base-toolchains%2Fdebug%2Cbb&revision=0cf10f5443cc6bfa4f9963aab7db8d5b840a0f21

failure log: https://treeherder.mozilla.org/logviewer?job_id=331679692&repo=autoland&lineNumber=32982

[task 2021-03-02T01:17:16.708Z] 01:17:16 INFO - make[4]: Entering directory '/builds/worker/workspace/obj-build/dom/gamepad'
[task 2021-03-02T01:17:16.710Z] 01:17:16 INFO - /builds/worker/fetches/sccache/sccache /builds/worker/fetches/gcc/bin/g++ -std=gnu++17 -o GamepadStateBroadcaster.o -c -I/builds/worker/workspace/obj-build/dist/stl_wrappers -I/builds/worker/workspace/obj-build/dist/system_wrappers -include /builds/worker/checkouts/gecko/config/gcc_hidden.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -DDEBUG=1 -DOS_POSIX=1 -DOS_LINUX=1 -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DSTATIC_EXPORTABLE_JS_API -I/builds/worker/checkouts/gecko/dom/gamepad -I/builds/worker/workspace/obj-build/dom/gamepad -I/builds/worker/checkouts/gecko/dom/gamepad/ipc -I/builds/worker/workspace/obj-build/ipc/ipdl/_ipdlheaders -I/builds/worker/checkouts/gecko/ipc/chromium/src -I/builds/worker/checkouts/gecko/ipc/glue -I/builds/worker/checkouts/gecko/dom/base -I/builds/worker/workspace/obj-build/dist/include -I/builds/worker/workspace/obj-build/dist/include/nspr -I/builds/worker/workspace/obj-build/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/obj-build/mozilla-config.h -Wall -Wempty-body -Wignored-qualifiers -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wduplicated-cond -Wimplicit-fallthrough -Wunused-function -Wunused-variable -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=coverage-mismatch -Wno-error=free-nonheap-object -Wformat -Wformat-overflow=2 -Wno-psabi -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -fno-aligned-new -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -freorder-blocks -Os -fno-omit-frame-pointer -funwind-tables -Werror -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/dbus-1.0 -I/usr/lib/x86_64-linux-gnu/dbus-1.0/include -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -MD -MP -MF .deps/GamepadStateBroadcaster.o.pp /builds/worker/checkouts/gecko/dom/gamepad/fallback/GamepadStateBroadcaster.cpp
[task 2021-03-02T01:17:16.711Z] 01:17:16 ERROR - /builds/worker/checkouts/gecko/dom/gamepad/fallback/GamepadStateBroadcaster.cpp:77:1: error: function 'mozilla::dom::GamepadStateBroadcaster::GamepadStateBroadcaster(mozilla::dom::GamepadStateBroadcaster&&)' defaulted on its redeclaration with an exception-specification that differs from the implicit exception-specification ''
[task 2021-03-02T01:17:16.711Z] 01:17:16 INFO - GamepadStateBroadcaster::GamepadStateBroadcaster(
[task 2021-03-02T01:17:16.711Z] 01:17:16 INFO - ^~~~~~~~~~~~~~~~~~~~~~~
[task 2021-03-02T01:17:16.712Z] 01:17:16 ERROR - /builds/worker/checkouts/gecko/dom/gamepad/fallback/GamepadStateBroadcaster.cpp:80:26: error: function 'mozilla::dom::GamepadStateBroadcaster& mozilla::dom::GamepadStateBroadcaster::operator=(mozilla::dom::GamepadStateBroadcaster&&)' defaulted on its redeclaration with an exception-specification that differs from the implicit exception-specification ''
[task 2021-03-02T01:17:16.712Z] 01:17:16 INFO - GamepadStateBroadcaster& GamepadStateBroadcaster::operator=(
[task 2021-03-02T01:17:16.712Z] 01:17:16 INFO - ^~~~~~~~~~~~~~~~~~~~~~~
[task 2021-03-02T01:17:16.712Z] 01:17:16 INFO - /builds/worker/checkouts/gecko/config/rules.mk:674: recipe for target 'GamepadStateBroadcaster.o' failed
[task 2021-03-02T01:17:16.713Z] 01:17:16 ERROR - make[4]: *** [GamepadStateBroadcaster.o] Error 1
[task 2021-03-02T01:17:16.713Z] 01:17:16 INFO - make[4]: Leaving directory '/builds/worker/workspace/obj-build/dom/gamepad'
[task 2021-03-02T01:17:16.713Z] 01:17:16 INFO - make[4]: *** Waiting for unfinished jobs....

Flags: needinfo?(cmartin)

Thanks Natalia. I did not see this error in my local build, Try build, or my static analysis build -- So it is very strange that this happened.

I will investigate and try again.

Flags: needinfo?(cmartin)
Pushed by cmartin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cb43cbbe1371
Create a class that implements sync'd cross-process shared memory r=handyman
https://hg.mozilla.org/integration/autoland/rev/e35d4c928753
Change PGamepadEventChannelParent to have a constructor r=handyman
https://hg.mozilla.org/integration/autoland/rev/6353d9bdcab7
Add a skeleton GamepadStateBroadcast r=handyman
https://hg.mozilla.org/integration/autoland/rev/c3ea9e47f68c
Use shared memory for gamepad state on Windows r=handyman

Backed out 4 changesets (bug 1658419) for GamepadStateBroadcaster related bustage.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&searchStr=build&fromchange=c3ea9e47f68c5478715bf320813e1f969dfd6973&selectedTaskRun=UYKnR4tRRVejS3umMF-bZg.0&tochange=c45b1e6bcd016bd6159170ad3d61523452c19218

Backout link: https://hg.mozilla.org/integration/autoland/rev/c45b1e6bcd016bd6159170ad3d61523452c19218

Failure log: https://treeherder.mozilla.org/logviewer?job_id=331829556&repo=autoland&lineNumber=9142

[task 2021-03-03T00:00:57.732Z] 00:00:57     INFO -  make[4]: Entering directory '/builds/worker/workspace/obj-build/dom/gamepad'
[task 2021-03-03T00:00:57.732Z] 00:00:57     INFO -  /builds/worker/fetches/sccache/sccache /builds/worker/fetches/clang/bin/clang++ --sysroot /builds/worker/fetches/sysroot -std=gnu++17 -o GamepadStateBroadcaster.o -c  -I/builds/worker/workspace/obj-build/dist/stl_wrappers -I/builds/worker/workspace/obj-build/dist/system_wrappers -include /builds/worker/checkouts/gecko/config/gcc_hidden.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -fstack-clash-protection -ftrivial-auto-var-init=pattern -DDEBUG=1 -DOS_POSIX=1 -DOS_LINUX=1 -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DSTATIC_EXPORTABLE_JS_API -I/builds/worker/checkouts/gecko/dom/gamepad -I/builds/worker/workspace/obj-build/dom/gamepad -I/builds/worker/checkouts/gecko/dom/gamepad/ipc -I/builds/worker/workspace/obj-build/ipc/ipdl/_ipdlheaders -I/builds/worker/checkouts/gecko/ipc/chromium/src -I/builds/worker/checkouts/gecko/ipc/glue -I/builds/worker/checkouts/gecko/dom/base -I/builds/worker/workspace/obj-build/dist/include -I/builds/worker/workspace/obj-build/dist/include/nspr -I/builds/worker/workspace/obj-build/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/obj-build/mozilla-config.h -Qunused-arguments -Qunused-arguments -Wall -Wbitfield-enum-conversion -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wempty-init-stmt -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wno-range-loop-analysis -Wc++2a-compat -Wcomma -Wimplicit-fallthrough -Wunused-function -Wunused-variable -Werror=non-literal-null-conversion -Wstring-conversion -Wtautological-overlap-compare -Wtautological-unsigned-enum-zero-compare -Wtautological-unsigned-zero-compare -Wno-error=tautological-type-limit-compare -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=backend-plugin -Wno-error=return-std-move -Wno-error=atomic-alignment -Wno-error=deprecated-copy -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-psabi -Wno-unknown-warning-option -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -fno-aligned-new -fcrash-diagnostics-dir=/builds/worker/artifacts -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -Xclang -load -Xclang /builds/worker/workspace/obj-build/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -Os -fno-omit-frame-pointer -funwind-tables -Werror -I/builds/worker/fetches/sysroot/usr/include/glib-2.0 -I/builds/worker/fetches/sysroot/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/builds/worker/fetches/sysroot/usr/include/dbus-1.0 -I/builds/worker/fetches/sysroot/usr/lib/x86_64-linux-gnu/dbus-1.0/include -I/builds/worker/fetches/sysroot/usr/include/glib-2.0 -I/builds/worker/fetches/sysroot/usr/lib/x86_64-linux-gnu/glib-2.0/include -fexperimental-new-pass-manager  -MD -MP -MF .deps/GamepadStateBroadcaster.o.pp   /builds/worker/checkouts/gecko/dom/gamepad/fallback/GamepadStateBroadcaster.cpp
[task 2021-03-03T00:00:57.732Z] 00:00:57    ERROR -  /builds/worker/checkouts/gecko/dom/gamepad/fallback/GamepadStateBroadcaster.cpp:77:26: error: 'GamepadStateBroadcaster' is missing exception specification 'noexcept'
[task 2021-03-03T00:00:57.732Z] 00:00:57     INFO -  GamepadStateBroadcaster::GamepadStateBroadcaster(
[task 2021-03-03T00:00:57.732Z] 00:00:57     INFO -                           ^
[task 2021-03-03T00:00:57.732Z] 00:00:57     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/dom/GamepadStateBroadcaster.h:126:3: note: previous declaration is here
[task 2021-03-03T00:00:57.732Z] 00:00:57     INFO -    GamepadStateBroadcaster(GamepadStateBroadcaster&& aOther) noexcept;
[task 2021-03-03T00:00:57.732Z] 00:00:57     INFO -    ^
[task 2021-03-03T00:00:57.732Z] 00:00:57    ERROR -  /builds/worker/checkouts/gecko/dom/gamepad/fallback/GamepadStateBroadcaster.cpp:80:51: error: 'operator=' is missing exception specification 'noexcept'
[task 2021-03-03T00:00:57.732Z] 00:00:57     INFO -  GamepadStateBroadcaster& GamepadStateBroadcaster::operator=(
[task 2021-03-03T00:00:57.732Z] 00:00:57     INFO -                                                    ^
[task 2021-03-03T00:00:57.732Z] 00:00:57     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/dom/GamepadStateBroadcaster.h:127:28: note: previous declaration is here
[task 2021-03-03T00:00:57.732Z] 00:00:57     INFO -    GamepadStateBroadcaster& operator=(GamepadStateBroadcaster&& aOther) noexcept;
[task 2021-03-03T00:00:57.733Z] 00:00:57     INFO -                             ^
[task 2021-03-03T00:00:57.733Z] 00:00:57     INFO -  2 errors generated.
[task 2021-03-03T00:00:57.734Z] 00:00:57    ERROR -  make[4]: *** [/builds/worker/checkouts/gecko/config/rules.mk:676: GamepadStateBroadcaster.o] Error 1
[task 2021-03-03T00:00:57.734Z] 00:00:57     INFO -  make[4]: Leaving directory '/builds/worker/workspace/obj-build/dom/gamepad'
[task 2021-03-03T00:00:57.736Z] 00:00:57    ERROR -  make[3]: *** [/builds/worker/checkouts/gecko/config/recurse.mk:72: dom/gamepad/target-objects] Error 2
[task 2021-03-03T00:00:57.736Z] 00:00:57     INFO -  make[3]: *** Waiting for unfinished jobs....
Flags: needinfo?(cmartin)

Alright, I'm giving up on the noexcept thing. It's a nice perf optimization, but apparently UniquePtr<T> is not ready for it, and I've failed to land this patch twice now because of it.

Attempting another landing now...

Flags: needinfo?(cmartin)
Pushed by cmartin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/11e3f0658049
Create a class that implements sync'd cross-process shared memory r=handyman
https://hg.mozilla.org/integration/autoland/rev/8d3c6d538b7b
Change PGamepadEventChannelParent to have a constructor r=handyman
https://hg.mozilla.org/integration/autoland/rev/7db6f9ba1fe6
Add a skeleton GamepadStateBroadcast r=handyman
https://hg.mozilla.org/integration/autoland/rev/d2688bd29cba
Use shared memory for gamepad state on Windows r=handyman

Thanks for the backout. I can see that this is going to take a bit more work before it can be landed...

Flags: needinfo?(cmartin)

I'm closing this bug. Unless something changes, the amount of work that may still be ahead of me to support this feature makes the benefits questionable. I'd also have to re-evaluate using IPDL to do this, since it's seen many improvements in the last couple of years.

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
Attachment #9203090 - Attachment is obsolete: true
Attachment #9203089 - Attachment is obsolete: true
Attachment #9194155 - Attachment is obsolete: true
Attachment #9203088 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: