Closed
Bug 1297804
Opened 6 years ago
Closed 6 years ago
eliminate unnecessary array bounds checks in IPDL-generated code
Categories
(Core :: IPC, defect)
Core
IPC
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: froydnj, Assigned: froydnj)
Details
Attachments
(5 files, 1 obsolete file)
2.15 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
3.67 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
3.49 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
1.12 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
3.37 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
These changes save ~80K or so on x86-64 Linux.
![]() |
Assignee | |
Comment 1•6 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•6 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•6 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•6 years ago
|
||
We know we're not going to perform out-of bounds accesses here.
Attachment #8784521 -
Flags: review?(wmccloskey)
![]() |
Assignee | |
Comment 5•6 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•6 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•6 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•6 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
Comment 13•6 years ago
|
||
bugherder |
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
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in
before you can comment on or make changes to this bug.
Description
•