1297804 - eliminate unnecessary array bounds checks in IPDL-generated code

Status: RESOLVED FIXED

Description

[Nathan Froyd [:froydnj] [away through 21 July]]

These changes save ~80K or so on x86-64 Linux.

Comment 1

[Nathan Froyd [:froydnj] [away through 21 July]]

Attachment: part 0 - add a RangedFor stament type to IPDL's code generator

This addition makes many of the upcoming patches significantly easier to
write, and enables us to avoid unpleasantness trying to fiddle with
ipdl.py's notions of C++ types.  (For instance, there's no good way,
when you have a type in-hand, to say the moral equivalent of
std::add_pointer<T>::type.)

Comment 2

[Nathan Froyd [:froydnj] [away through 21 July]]

Attachment: part 1 - eschew array bounds checking in nsTArray IPDL-generated code, reader-side

For better or worse, the IPDL compiler, for protocols that send or
receive arrays, generates Read methods for transferring those arrays,
rather than going through the standard ParamTraits specializations.
These generated methods perform unnecessary bounds checks when accessing
elements; such checks can be safely bypassed because we know the exact
length of all the arrays involved.  (Some compilers are not smart enough
to eliminate the bounds checks on their own.)

Comment 3

[Nathan Froyd [:froydnj] [away through 21 July]]

Attachment: part 2 - eschew array bounds checking in nsTArray IPDL-generated code, writer-side

For better or worse, the IPDL compiler, for protocols that send or
receive arrays, generates Write methods for transferring those arrays,
rather than going through the standard ParamTraits specializations.
These generated methods perform unnecessary bounds checks when accessing
elements; such checks can be safely bypassed because we know the exact
length of all the arrays involved.  (Some compilers are not smart enough
to eliminate the bounds checks on their own.)

Comment 4

[Nathan Froyd [:froydnj] [away through 21 July]]

Attachment: part 3 - avoid array bounds checking when writing nsTArray<T> to messages

We know we're not going to perform out-of bounds accesses here.

Comment 5

[Nathan Froyd [:froydnj] [away through 21 July]]

Attachment: part 4 - avoid array bounds checks in DestroySubtree loops

The IPDL compiler generates code like this in DestroySubtree methods:

    // Recursively shutting down PAPZ kids
    nsTArray<PAPZChild*> kids((mManagedPAPZChild).Count());
    // Accumulate kids into a stable structure to iterate over
    ManagedPAPZChild(kids);
    for (uint32_t i = 0; (i) < ((kids).Length()); (++(i))) {
        // Guarding against a child removing a sibling from the list during the iteration.
        if ((mManagedPAPZChild).Contains(kids[i])) {
            (kids[i])->DestroySubtree(subtreewhy);
        }
    }

There are three problems with this code:

1) We initialize |kids| with a capacity, which is wasted work;
   ManagedPAPZChild() is going to resize the array for us anyway.
2) We're constantly reading from |kids.Length()| in the loop, when we
   only really need to read from it once.
3) We're repeatedly accessing kids[i], and doing needless bounds checks.

Let's fix those three problems.

Comment 6

[Nathan Froyd [:froydnj] [away through 21 July]]

Attachment: part 5 - avoid array bounds checking in IPDL-generated CloneManagees functions

CloneManagees functions contain loops that look like:

    nsTArray<PAPZParent*> kids;
    (other)->ManagedPAPZParent(kids);
    PAPZParent* actor;
    for (uint32_t i = 0; (i) < ((kids).Length()); (++(i))) {
        actor = static_cast<PAPZParent*>((kids[i])->CloneProtocol((&(mChannel)), aCtx));
        if ((actor) == (nullptr)) {
            FatalError("can not clone an PAPZ actor");
            return;
        }
        (actor)->mId = (kids[i])->mId;
        (actor)->mManager = this;
        (actor)->mChannel = (&(mChannel));
        (actor)->mState = (kids[i])->mState;
        (mManagedPAPZParent).PutEntry(actor);
        RegisterID(actor, (actor)->mId);
        (actor)->CloneManagees(kids[i], aCtx);
    }

which is inefficient in every way that the DestroySubtree loops were,
and exacerbated by containing even more references to |kids[i]|.  The
fix is the same as for DestroySubtrees.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8784520 [details] [diff] [review]
part 2 - eschew array bounds checking in nsTArray IPDL-generated code, writer-side

Review of attachment 8784520 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/ipdl/ipdl/lower.py
@@ +4436,2 @@
>          directtype = Type(directtype.name, **typeinit)
> +        constdirecttype = Type(directtype.name, **consttypeinit)

I'm having trouble understing this. constdirecttype appears to be dead (and this consttypeinit as well). I have to admit that it's hard to understand what's going on here without seeing the generated code, though.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8784522 [details] [diff] [review]
part 4 - avoid array bounds checks in DestroySubtree loops

Review of attachment 8784522 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/ipdl/ipdl/lower.py
@@ -3471,4 @@
>  
>              foreachdestroy.addstmt(
>                  Whitespace('// Guarding against a child removing a sibling from the list during the iteration.\n', indent=1))
> -            ifhas = StmtIf(_callHasManagedActor(managedVar, ithkid))

Can you delete the assignment to ithkid above? Seems like it's dead now.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8784523 [details] [diff] [review]
part 5 - avoid array bounds checking in IPDL-generated CloneManagees functions

Review of attachment 8784523 [details] [diff] [review]:
-----------------------------------------------------------------

CloneManagees is part of Nuwa, which was removed in bug 1284674 (hurrah!). I guess CloneManagees got missed. I'm fine with whatever you want to do here. But if you're feeling a bit more ambitious, I'll happily rubber stamp a patch to remove anything defining or using CloneManagees or CloneToplevel (as well as anything else calling those, recursively). It will definitely save some space in libxul.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Also, thanks for all the great work in this area!

Comment 11

[Nathan Froyd [:froydnj] [away through 21 July]]

Comment on attachment 8784523 [details] [diff] [review]
part 5 - avoid array bounds checking in IPDL-generated CloneManagees functions

Oh, that's excellent.  I'll just drop this part and move the removal of the dead code to a different bug.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1477881ee551
part 0 - add a RangedFor stament type to IPDL's code generator; r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad48a00323b3
part 1 - eschew array bounds checking in nsTArray IPDL-generated code, reader-side; r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/92bd6c9ee5f5
part 2 - eschew array bounds checking in nsTArray IPDL-generated code, writer-side; r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/af44291ba592
part 3 - avoid array bounds checking when writing nsTArray<T> to messages; r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b874fea3cbc
part 4 - avoid array bounds checks in DestroySubtree loops; r=billm

Comment 13

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/1477881ee551
https://hg.mozilla.org/mozilla-central/rev/ad48a00323b3
https://hg.mozilla.org/mozilla-central/rev/92bd6c9ee5f5
https://hg.mozilla.org/mozilla-central/rev/af44291ba592
https://hg.mozilla.org/mozilla-central/rev/1b874fea3cbc