1479996 - micro-optimize frame properties - avoid ptr->index->ptr dance

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P3)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Bug 1478138 introduced an optimization in FrameProperties::{G,S}etInternal where following nsTArray::IndexOf, we know the returned index *must* be valid, so it is more efficient to use nsTArray::operator[] instead of ElementAt (the latter does a bound check).

In bug 1478138 comment 4 I suggested a further optimization:
Because IndexOf converts a pointer in the array to an index, and then operator[] converts the index back into the original pointer, it would be more efficient to directly get a pointer from an IndexOf-like function.
Or, for better safety, the IndexOf-like function could be given lambdas, one to call with the found element and the other one if nothing is found.

Simple proof of concept showing the apparent benefit in generated code:
https://godbolt.org/g/NfX81k
E.g., results with clang 6.0.0 -O3: Something like FrameProperties::GetInternal() takes 32 instructions with IndexOf+operator[], only 23 instructions with lambda-friendly function.

This could benefit other users of nsTArray, if they have similar needs.


Note: Optimizing RemoveInternal is trickier and probably won't be touched in this bug.

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1479996 - Combine nsTArray::IndexOf and element access into lambda-friendly functions - r?froydnj

In many places, nsTArray::IndexOf is followed by accessing that element
(hopefully with `Elements() + index`, which skips unnecessary bounds checks.)
But this pattern introduces operations that could be avoided:
- IndexOf converts the address of the found element into an index,
- The caller must test for a special `NoIndex` value,
- On success, accesses convert that index back into the original address.

This patch introduces new 'ApplyIf...' functions that combine all these
operations in a more efficient ensemble: If the sought element is found, it is
passed by reference to a given callable object (usually a lambda); if not
found, another callable is invoked.
On top of removing the pointer->index->pointer operations, invoking callables
directly from ApplyIf is safer, as the array is guaranteed to be untouched at
this time.
Also, being templates taking function objects, in most cases the compiler should
be able to inline and optimize the search and its callables' code.

This patch gives example uses in nsTArray::Contains, and in
FrameProperties::GetInternal, SetInternal.
And FrameProperties::Has now calls Contains, which is more efficient.

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8997764 [details]
Bug 1479996 - Combine nsTArray::IndexOf and element access into lambda-friendly functions - r?froydnj

Nathan Froyd [:froydnj] has approved the revision.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9fa92a1d961d
Combine nsTArray::IndexOf and element access into lambda-friendly functions - r=froydnj

Comment 4

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/9fa92a1d961d