1553731 - avoid reading from the "frame class to frame type" map when we can

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P3)

Description

[Cameron McCormack (:heycam)]

We do many nsIFrame::Type() checks. That function needs to look up an array to map the nsQueryFrame::ClassID to a LayoutFrameType. In many cases, we just want to check if it is equal to one or more specific types. By sorting the ClassID enum values, grouping by their corresponding LayoutFrameType, we can make Type() checks cheaper.

Comment 1

[Cameron McCormack (:heycam)]

There are 11 LayoutFrameTypes that correspond to more than one ClassID:

• None: nsBox, nsFrame, nsHTMLFramesetBlankFrame, nsHTMLFramesetBorderFrame, nsMathMLFrame, nsMathMLmactionFrame, nsMathMLmencloseFrame, nsMathMLmfencedFrame, nsMathMLmfracFrame, nsMathMLmmultiscriptsFrame, nsMathMLmoFrame, nsMathMLmpaddedFrame, nsMathMLmrootFrame, nsMathMLmrowFrame, nsMathMLmspaceFrame, nsMathMLmsqrtFrame, nsMathMLmunderoverFrame, nsMathMLsemanticsFrame, nsMathMLTokenFrame, nsSVGContainerFrame
• Box: nsBoxFrame, nsButtonBoxFrame, nsDocElementBoxFrame, nsGridRowGroupFrame, nsGridRowLeafFrame, nsGroupBoxFrame, nsMenuBarFrame, nsResizerFrame, nsScrollbarButtonFrame, nsSplitterFrame, nsStackFrame, nsTitleBarFrame, nsTreeColFrame
• Block: nsBlockFrame, nsFileControlFrame, nsMathMLmathBlockFrame, nsMathMLmtdInnerFrame, nsSelectsAreaFrame
• LeafBox: nsLeafBoxFrame, nsTextBoxFrame, nsTreeBodyFrame
• Inline: nsInlineFrame, nsMathMLmathInlineFrame
• Scroll: nsHTMLScrollFrame, nsXULScrollFrame
• Table: nsMathMLmtableFrame, nsTableFrame
• TableCell: nsMathMLmtdFrame, nsTableCellFrame
• TableRow: nsMathMLmtrFrame, nsTableRowFrame
• TableWrapper: nsMathMLmtableWrapperFrame, nsTableWrapperFrame
• Text: nsContinuingTextFrame, nsTextFrame

and the remaining 98 LayoutFrameTypes correspond to a single ClassID.

Comment 2

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=920bb5696d35200472d29a21281206006f33576e

These patches drop the size of my local libxul.so by about 30 KiB.

Comment 3

[Cameron McCormack (:heycam)]

Lots of orange to fix first though...

Comment 4

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=31df8b37df6fd0edb0d8627106c17d0221d23943

Comment 5

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #2)

These patches drop the size of my local libxul.so by about 30 KiB.

Actually only 8 KiB after stripping.

Comment 6

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e0de058db7c9dc8d680e127ae6d51bbf05795f8

Comment 7

[Cameron McCormack (:heycam)]

Attachment: Bug 1553731 - Part 1: Rename nsFrameIdList.h to FrameIdList.h.

Comment 8

[Cameron McCormack (:heycam)]

Attachment: Bug 1553731 - Part 2: Generate FrameIdList.h and FrameTypeList.h from Python.

Comment 9

[Cameron McCormack (:heycam)]

Attachment: Bug 1553731 - Part 3: Make nsIFrame::Is___Frame() avoid reading from memory.

Comment 10

[Cameron McCormack (:heycam)]

Attachment: Bug 1553731 - Part 4: Replace many nsIFrame::Type() calls with specific nsIFrame::Is___Frame() calls.

Comment 11

[Mats Palmgren (inactive)]

Removing the Type() table lookup is great, so part 1 to 3 seems fine to me.

I don't think we should do part 4 though, because I think we should remove
Type() eventually so I'd like leave those calls as is to be easily audit-able.
We should audit them manually and decide how we can remove them.
Some cases might want to use do_QueryFrame instead, or if the source of
the problem is an nsIFrame* param (not unusual) we should consider
using the subclass type directly instead (i.e. nsBlockFrame* aBlock).
Granted, in some cases we might want to use Is__Frame(), but long
term, I think both Type() and Is__Frame() should be removed in favor of
proper types, do_QueryFrame where needed, or some other mechanism.
Type(), and anything built on it, is rather fragile and foot-gunny.

Comment 12

[Cameron McCormack (:heycam)]

OK, would you mind filing a bug if there isn't one for the Type() auditing? It should be possible to redesign do_QueryFrame to avoid a virtual call and do some quick determinations based on the mClass like the Type() calls.

Comment 13

[Mats Palmgren (inactive)]

OK, filed bug 1555664 for the Type() auditing.

FYI, do_QueryFrame already avoids the virtual call when the target type is a final class,
but yes we could also use Type() internally to optimize dQF to nsBlockFrame etc.

The problem with Type() is that it encodes class inheritance out-of-band, which is brittle.
It wouldn't surprise me if Frame("nsBlockFrame", "Table", NOT_LEAF) compiles,
for example. While we can continue to rely on reviewers to catch such errors, it would
be nice if we could also generate some code that would fail to compile such cases.

So, I think we should:

1. audit Type(), remove it as much as possible (bug 1555664)
2. convert remaining cases to Is__Frame() (as your part 4 does here)
3. rename Is__Frame() to Is__FrameOrSubclass() to emphasize what it does
4. implement Is__FrameOrSubclass() using dQF, same as we've manually done here:
   https://searchfox.org/mozilla-central/source/layout/generic/nsFrame.cpp#7484
   (BTW, do we have any ns<T>Frame subclasses that doesn't have
   a "<T>" Type() for some reason? (I hope not, but probably worth checking.))
5. optimize dQF to use Type() for specific non-final target types such as nsBlockFrame etc
6. make Type() private

How does that sound?

We should probably also expose mClass in some way to make it possible to test
which specific class an instance is, at least as #ifdef DEBUG for assertions.
Something like frame->IsInstanceOf<nsBlockFrame>(), which would test
frame->mClass == nsBlockFrame::kFrameIID.

Comment 14

[Cameron McCormack (:heycam)]

That sounds good to me, Mats.

Comment 15

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1971a43f6295f5b199281c4d0937dd1b8d902674

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0d0bf5d84a3c
Part 1: Rename nsFrameIdList.h to FrameIdList.h. r=mats
https://hg.mozilla.org/integration/autoland/rev/b8669aa5a820
Part 2: Generate FrameIdList.h and FrameTypeList.h from Python. r=mats,glandium
https://hg.mozilla.org/integration/autoland/rev/90ed9808e1c0
Part 3: Make nsIFrame::Is___Frame() avoid reading from memory. r=mats

Comment 17

[Cosmin Sabou [:CosminS]]

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=busted&revision=90ed9808e1c0bc5216022e15c32a7eb3704ce20d

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=249515332&repo=autoland&lineNumber=12880

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

[task 2019-06-01T01:15:19.420Z] 01:15:19 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/security/sandbox/linux/broker'
[task 2019-06-01T01:15:19.420Z] 01:15:19 INFO - /builds/worker/workspace/build/src/sccache/sccache /builds/worker/workspace/build/src/gcc/bin/g++ -o Unified_cpp_linux_broker0.o -c -I/builds/worker/workspace/build/src/obj-firefox/dist/stl_wrappers -I/builds/worker/workspace/build/src/obj-firefox/dist/system_wrappers -include /builds/worker/workspace/build/src/config/gcc_hidden.h -DDEBUG=1 -DOS_POSIX=1 -DOS_LINUX=1 -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/builds/worker/workspace/build/src/security/sandbox/linux/broker -I/builds/worker/workspace/build/src/obj-firefox/security/sandbox/linux/broker -I/builds/worker/workspace/build/src/security/sandbox/linux -I/builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/_ipdlheaders -I/builds/worker/workspace/build/src/ipc/chromium/src -I/builds/worker/workspace/build/src/ipc/glue -I/builds/worker/workspace/build/src/security/sandbox/chromium -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/build/src/obj-firefox/mozilla-config.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -Wall -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wduplicated-cond -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=coverage-mismatch -Wno-error=free-nonheap-object -Wformat -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -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/builds/worker/workspace/build/src/widget/gtk/compat-gtk3 -pthread -I/usr/include/gtk-3.0 -I/usr/include/pango-1.0 -I/usr/include/gio-unix-2.0/ -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/freetype2 -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/libpng12 -I/usr/include/gtk-3.0/unix-print -MD -MP -MF .deps/Unified_cpp_linux_broker0.o.pp /builds/worker/workspace/build/src/obj-firefox/security/sandbox/linux/broker/Unified_cpp_linux_broker0.cpp
[task 2019-06-01T01:15:19.420Z] 01:15:19 INFO - In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/WheelHandlingHelper.h:13:0,
[task 2019-06-01T01:15:19.420Z] 01:15:19 INFO - from /builds/worker/workspace/build/src/obj-firefox/dist/include/ipc/nsGUIEventIPC.h:17,
[task 2019-06-01T01:15:19.420Z] 01:15:19 INFO - from /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/ipdlheaders/mozilla/dom/PBrowserBridgeChild.h:19,
[task 2019-06-01T01:15:19.420Z] 01:15:19 INFO - from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/BrowserBridgeChild.h:10,
[task 2019-06-01T01:15:19.420Z] 01:15:19 INFO - from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/ContentChild.h:13,
[task 2019-06-01T01:15:19.421Z] 01:15:19 INFO - from /builds/worker/workspace/build/src/security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp:19,
[task 2019-06-01T01:15:19.421Z] 01:15:19 INFO - from /builds/worker/workspace/build/src/obj-firefox/security/sandbox/linux/broker/Unified_cpp_linux_broker0.cpp:20:
[task 2019-06-01T01:15:19.421Z] 01:15:19 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/FrameTypeList.h: In member function 'bool nsIFrame::IsNoneFrame() const':
[task 2019-06-01T01:15:19.422Z] 01:15:19 ERROR - /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIFrame.h:2800:28: error: comparison is always true due to limited range of data type [-Werror=type-limits]
[task 2019-06-01T01:15:19.423Z] 01:15:19 INFO - return uint8_t(mClass) >= uint8_t(ClassID::first_class##id) &&
[task 2019-06-01T01:15:19.424Z] 01:15:19 INFO - ^
[task 2019-06-01T01:15:19.424Z] 01:15:19 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIFrame.h:2800:28: note: in definition of macro 'FRAME_TYPE'
[task 2019-06-01T01:15:19.424Z] 01:15:19 INFO - return uint8_t(mClass) >= uint8_t(ClassID::first_class##_id) &&
[task 2019-06-01T01:15:19.424Z] 01:15:19 INFO - ^~
[task 2019-06-01T01:15:19.425Z] 01:15:19 INFO - cc1plus: all warnings being treated as errors
[task 2019-06-01T01:15:19.425Z] 01:15:19 INFO - /builds/worker/workspace/build/src/config/rules.mk:810: recipe for target 'Unified_cpp_linux_broker0.o' failed
[task 2019-06-01T01:15:19.425Z] 01:15:19 ERROR - make[4]: *** [Unified_cpp_linux_broker0.o] Error 1
[task 2019-06-01T01:15:19.425Z] 01:15:19 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/security/sandbox/linux/broker'
[task 2019-06-01T01:15:19.425Z] 01:15:19 INFO - /builds/worker/workspace/build/src/config/recurse.mk:74: recipe for target 'security/sandbox/linux/broker/target' failed
[task 2019-06-01T01:15:19.425Z] 01:15:19 ERROR - make[3]: *** [security/sandbox/linux/broker/target] Error 2
[task 2019-06-01T01:15:19.425Z] 01:15:19 INFO - make[3]: *** Waiting for unfinished jobs....

Comment 18

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dd63c32f5ae5cb78397e9cb7c6deda2161e676f

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5b78e2920004
Part 1: Rename nsFrameIdList.h to FrameIdList.h. r=mats
https://hg.mozilla.org/integration/autoland/rev/eec771e04586
Part 2: Generate FrameIdList.h and FrameTypeList.h from Python. r=mats,glandium
https://hg.mozilla.org/integration/autoland/rev/2da22c064717
Part 3: Make nsIFrame::Is___Frame() avoid reading from memory. r=mats

Comment 20

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/5b78e2920004
https://hg.mozilla.org/mozilla-central/rev/eec771e04586
https://hg.mozilla.org/mozilla-central/rev/2da22c064717