1962217 - Refactor NativeLayerCA surface management methods to a helper class

Status: RESOLVED FIXED

(Core :: Graphics, task)

Description

[Brad Werth [:bradwerth]]

NativeLayer has a few methods that return surface handles for drawing, etc. These methods are overridden by subclasses like NativeLayerCA with platform-specific implementations (macOS in this case). In order to implement GPU process on macOS, we'll need a new subclass of NativeLayer that will manage the macOS surfaces on the GPU side, without including the transaction management done by NativeLayerCA. It would be useful to refactor the implementation of these methods so they could be shared by the new subclass.

Comment 1

[Brad Werth [:bradwerth]]

Attachment: Bug 1962217 Part 1: Refactor NativeLayerCA into a helper class for surface management.

Comment 2

[Brad Werth [:bradwerth]]

Attachment: Bug 1962217 Part 2: Make NativeLayerCA use the helper class for surface management.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d46f772683e8
Part 1: Refactor NativeLayerCA into a helper class for surface management. r=mstange
https://hg.mozilla.org/integration/autoland/rev/27ea4d72c03f
Part 2: Make NativeLayerCA use the helper class for surface management. r=mstange

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9c23e4cc0fab
Revert "Bug 1962217 Part 2: Make NativeLayerCA use the helper class for surface management. r=mstange"
https://hg.mozilla.org/integration/autoland/rev/45c354220de1
Revert "Bug 1962217 Part 1: Refactor NativeLayerCA into a helper class for surface management. r=mstange"

Comment 5

[Iulian Moraru]

Revert for causing non unified build bustages.

• Push with failures

• Failure log

• Backout link part 1

• Backout link part 2

[task 2025-05-15T03:06:00.294Z] 03:06:00     INFO -  /builds/worker/fetches/sccache/sccache /builds/worker/fetches/clang/bin/clang++ -isysroot /builds/worker/fetches/MacOSX15.4.sdk -mmacosx-version-min=10.15 -stdlib=libc++ --target=x86_64-apple-darwin -o NativeLayerMacSurfaceHandler.o -c  -I/builds/worker/workspace/obj-build/dist/stl_wrappers -fvisibility=hidden -fvisibility-inlines-hidden -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -fstrict-flex-arrays=1 -DNDEBUG=1 -DTRIMMED=1 -DGOOGLE_PROTOBUF_NO_RTTI -DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DMOZ_APP_VERSION=140.0a1 -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DMOZ_SUPPORT_LEAKCHECKING -DSTATIC_EXPORTABLE_JS_API -I/builds/worker/checkouts/gecko/gfx/layers -I/builds/worker/workspace/obj-build/gfx/layers -I/builds/worker/workspace/obj-build/ipc/ipdl/_ipdlheaders -I/builds/worker/checkouts/gecko/ipc/chromium/src -I/builds/worker/checkouts/gecko/docshell/base -I/builds/worker/checkouts/gecko/dom/canvas -I/builds/worker/checkouts/gecko/gfx/cairo/cairo/src -I/builds/worker/checkouts/gecko/layout/base -I/builds/worker/checkouts/gecko/layout/generic -I/builds/worker/checkouts/gecko/media/libyuv/libyuv/include -I/builds/worker/checkouts/gecko/gfx/skia -I/builds/worker/checkouts/gecko/gfx/skia/skia -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 -DMOZILLA_CLIENT -include /builds/worker/workspace/obj-build/mozilla-config.h -fno-rtti -pthread -fno-sized-deallocation -fno-aligned-new -ffunction-sections -fdata-sections -fno-math-errno -fno-exceptions -fPIC -fcrash-diagnostics-dir=/builds/worker/artifacts -gdwarf-4 -Xclang -load -Xclang /builds/worker/workspace/obj-build/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O3 -fno-omit-frame-pointer -funwind-tables -Werror -Wall -Wbitfield-enum-conversion -Wempty-body -Wformat-type-confusion -Wignored-qualifiers -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtautological-constant-in-range-compare -Wtype-limits -Wno-error=tautological-type-limit-compare -Wunreachable-code -Wunreachable-code-return -Wunused-but-set-parameter -Wno-invalid-offsetof -Wclass-varargs -Wempty-init-stmt -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wno-range-loop-analysis -Wenum-compare-conditional -Wenum-float-conversion -Wno-deprecated-anon-enum-enum-conversion -Wno-deprecated-enum-enum-conversion -Wno-deprecated-this-capture -Wcomma -Wimplicit-fallthrough -Wduplicate-method-arg -Wduplicate-method-match -Wmissing-method-return-type -Wobjc-signed-char-bool -Wsemicolon-before-method-body -Wsuper-class-method-mismatch -Wstring-conversion -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=free-nonheap-object -Wno-error=atomic-alignment -Wno-error=deprecated-builtins -Wformat -Wformat-security -Wno-psabi -Wthread-safety -Werror=unguarded-availability-new -Wno-error=builtin-macro-redefined -Wno-vla-cxx-extension -Wno-unknown-warning-option -Werror=switch -fno-strict-aliasing -ffp-contract=off  -MD -MP -MF .deps/NativeLayerMacSurfaceHandler.o.pp   -x objective-c++ -fobjc-exceptions   /builds/worker/checkouts/gecko/gfx/layers/NativeLayerMacSurfaceHandler.mm
[task 2025-05-15T03:06:00.294Z] 03:06:00     INFO -  In file included from /builds/worker/checkouts/gecko/gfx/layers/NativeLayerMacSurfaceHandler.mm:6:
[task 2025-05-15T03:06:00.294Z] 03:06:00    ERROR -  /builds/worker/workspace/obj-build/dist/include/mozilla/layers/NativeLayerMacSurfaceHandler.h:31:16: error: use of undeclared identifier 'IOSurfaceRef'
[task 2025-05-15T03:06:00.294Z] 03:06:00     INFO -     31 |   CFTypeRefPtr<IOSurfaceRef> mSurface;
[task 2025-05-15T03:06:00.294Z] 03:06:00     INFO -        |                ^
[task 2025-05-15T03:06:00.294Z] 03:06:00    ERROR -  /builds/worker/workspace/obj-build/dist/include/mozilla/layers/NativeLayerMacSurfaceHandler.h:105:15: error: no member named 'DrawTarget' in namespace 'mozilla::gfx'
[task 2025-05-15T03:06:00.294Z] 03:06:00     INFO -    105 |   RefPtr<gfx::DrawTarget> NextSurfaceAsDrawTarget(
[task 2025-05-15T03:06:00.294Z] 03:06:00     INFO -        |          ~~~~~^
[task 2025-05-15T03:06:00.294Z] 03:06:00     INFO -  In file included from /builds/worker/checkouts/gecko/gfx/layers/NativeLayerMacSurfaceHandler.mm:6:
[task 2025-05-15T03:06:00.294Z] 03:06:00     INFO -  In file included from /builds/worker/workspace/obj-build/dist/include/mozilla/layers/NativeLayerMacSurfaceHandler.h:9:
[task 2025-05-15T03:06:00.294Z] 03:06:00    ERROR -  /builds/worker/workspace/obj-build/dist/include/mozilla/Maybe.h:378:34: error: 'mIsSome' is a protected member of 'mozilla::detail::MaybeStorage<mozilla::layers::SurfaceWithInvalidRegion>'
[task 2025-05-15T03:06:00.294Z] 03:06:00     INFO -    378 |   using detail::MaybeStorage<T>::mIsSome;
[task 2025-05-15T03:06:00.294Z] 03:06:00     INFO -        |                                  ^
[task 2025-05-15T03:06:00.295Z] 03:06:00     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/layers/NativeLayerMacSurfaceHandler.h:207:35: note: in instantiation of template class 'mozilla::Maybe<mozilla::layers::SurfaceWithInvalidRegion>' requested here
[task 2025-05-15T03:06:00.295Z] 03:06:00     INFO -    207 |   Maybe<SurfaceWithInvalidRegion> mInProgressSurface;
[task 2025-05-15T03:06:00.295Z] 03:06:00     INFO -        |                                   ^
[task 2025-05-15T03:06:00.295Z] 03:06:00     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/Maybe.h:250:8: note: declared protected here
[task 2025-05-15T03:06:00.295Z] 03:06:00     INFO -    250 |   char mIsSome = false;  // not bool -- guarantees minimal space consumption
[task 2025-05-15T03:06:00.295Z] 03:06:00     INFO -        |        ^
[task 2025-05-15T03:06:00.295Z] 03:06:00     INFO -  In file included from /builds/worker/checkouts/gecko/gfx/layers/NativeLayerMacSurfaceHandler.mm:6:
[task 2025-05-15T03:06:00.295Z] 03:06:00    ERROR -  /builds/worker/workspace/obj-build/dist/include/mozilla/layers/NativeLayerMacSurfaceHandler.h:222:10: error: use of undeclared identifier 'MacIOSurface'
[task 2025-05-15T03:06:00.295Z] 03:06:00     INFO -    222 |   RefPtr<MacIOSurface> mInProgressLockedIOSurface;
[task 2025-05-15T03:06:00.296Z] 03:06:00     INFO -        |          ^
[task 2025-05-15T03:06:00.296Z] 03:06:00    ERROR -  /builds/worker/checkouts/gecko/gfx/layers/NativeLayerMacSurfaceHandler.mm:38:5: error: use of undeclared identifier 'gfxCriticalError'
[task 2025-05-15T03:06:00.296Z] 03:06:00     INFO -     38 |     gfxCriticalError()

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/98b5280f31f9
Part 1: Refactor NativeLayerCA into a helper class for surface management. r=mstange
https://hg.mozilla.org/integration/autoland/rev/8dbedafe63f7
Part 2: Make NativeLayerCA use the helper class for surface management. r=mstange

Comment 7

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/98b5280f31f9
https://hg.mozilla.org/mozilla-central/rev/8dbedafe63f7

Comment 8

[Alex Finder|PTO, returning August 18th]

(In reply to Pulsebot from comment #4)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9c23e4cc0fab
Revert "Bug 1962217 Part 2: Make NativeLayerCA use the helper class for
surface management. r=mstange"
https://hg.mozilla.org/integration/autoland/rev/45c354220de1
Revert "Bug 1962217 Part 1: Refactor NativeLayerCA into a helper class for
surface management. r=mstange"

Perfherder has detected a talos performance change from push 45c354220de172c3b1bfb2cb8e6cc40ad0947bd2. As author of one of the patches included in that push, we need your help to address this regression.

Please acknowledge, and begin investigating this alert within 3 business days, or the patch(es) may be backed out in accordance with our regression policy. Our guide to handling regression bugs has information about how you can proceed with this investigation.

If you have any questions or need any help with the investigation, please reach out to a performance sheriff. Alternatively, you can find help on Slack by joining #perf-help, and on Matrix you can find help by joining #perftest.

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)
11%	perf_reftest_singletons parent-basic-singleton.html	macosx1470-64-shippable	e10s fission stylo webrender	78.72 -> 87.00

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a performance sheriff to do that for you.

You can run all of these tests on try with ./mach try perf --alert 45217

The following documentation link provides more information about this command.

Comment 9

[Brad Werth [:bradwerth]]

Since the performance regression (if it's real) was caused by a backout of these patches, which have since landed, I'm not sure there's any action to take. If there's a new performance regression from the landed patches, then I'll work that. I'm going to take the perf-alert keyword off.