eliminate unnecessary array bounds checks in IPDL-generated code

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

Assignee

Description

3 years ago
These changes save ~80K or so on x86-64 Linux.
Assignee

Comment 1

3 years ago
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.)
Attachment #8784518 - Flags: review?(wmccloskey)
Assignee

Comment 2

3 years ago
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.)
Attachment #8784519 - Flags: review?(wmccloskey)
Assignee

Comment 3

3 years ago
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.)
Attachment #8784520 - Flags: review?(wmccloskey)
Assignee

Comment 4

3 years ago
We know we're not going to perform out-of bounds accesses here.
Attachment #8784521 - Flags: review?(wmccloskey)
Assignee

Comment 5

3 years ago
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.
Attachment #8784522 - Flags: review?(wmccloskey)
Assignee

Comment 6

3 years ago
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.
Attachment #8784523 - Flags: review?(wmccloskey)
Attachment #8784518 - Flags: review?(wmccloskey) → review+
Attachment #8784519 - Flags: review?(wmccloskey) → review+
Attachment #8784521 - Flags: review?(wmccloskey) → review+
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.
Attachment #8784520 - Flags: review?(wmccloskey) → review+
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.
Attachment #8784522 - Flags: review?(wmccloskey) → review+
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.
Attachment #8784523 - Flags: review?(wmccloskey) → review+
Also, thanks for all the great work in this area!
Assignee

Comment 11

3 years ago
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.
Attachment #8784523 - Attachment is obsolete: true

Comment 12

3 years ago
Pushed by nfroyd@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.