Eliminate virtual calls in xpc::AccessCheck methods

RESOLVED FIXED in Firefox 55

Status

()

Core
XPConnect
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: Away for a while, Assigned: Away for a while)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

8 months ago
Follow-up from bug 1340710.  Bobby, is this what you meant in bug 1340710 comment 77?
(Assignee)

Updated

8 months ago
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 1

8 months ago
Also, I'd like to try to inline some code here as well.  What do you think about that?  I'm asking because sometimes I get push back due to adding code in headers and #include dependencies and whatnot.
I was specifically talking about the nsIPrincipal methods called in xpc::WrapperFactory::Rewrap [1], which end up in AccessCheck via things like [2].

All those can be replaced with BasePrincipal::Cast(prin)->Fast{Equals,Subsumes}{,ConsideringDomain}.

I am fine with inlining code here, and indeed would be interested to see that. I don't care about includes as long as it's not a crazy number of them.

[1] http://searchfox.org/mozilla-central/rev/1bc7720a434af3c13b68b8d69f92078cae8890c4/js/xpconnect/wrappers/WrapperFactory.cpp#466
[2] http://searchfox.org/mozilla-central/rev/1bc7720a434af3c13b68b8d69f92078cae8890c4/js/xpconnect/wrappers/AccessCheck.cpp#66
Flags: needinfo?(bobbyholley)
(Assignee)

Updated

8 months ago
Assignee: nobody → ehsan
(Assignee)

Comment 3

8 months ago
Created attachment 8844315 [details] [diff] [review]
Part 1: Factor out more non-virtual helpers for principal equality/subsumption checks
Attachment #8844315 - Flags: review?(bobbyholley)
(Assignee)

Comment 4

8 months ago
Created attachment 8844316 [details] [diff] [review]
Part 2: Make the non-virtual helpers for principal equality/subsumption checks inline
Attachment #8844316 - Flags: review?(bobbyholley)
(Assignee)

Comment 5

8 months ago
Created attachment 8844317 [details] [diff] [review]
Part 3: Speed up principal access checks in WrapperFactory::Rewrap() by eliminating virtual dispatch and inling
Attachment #8844317 - Flags: review?(bobbyholley)
Comment on attachment 8844315 [details] [diff] [review]
Part 1: Factor out more non-virtual helpers for principal equality/subsumption checks

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

::: caps/BasePrincipal.h
@@ +290,3 @@
>    // Call this instead of Subsumes to avoid the const of virtual dispatch.
>    bool FastSubsumes(nsIPrincipal* aOther);
> +  // Call this instead of SubsumesConsideringDomain to avoid the const of virtual dispatch.

Nit: "cost" rather than "const", here and below. But please just have one comment saying "Call these" and group them together.
Attachment #8844315 - Flags: review?(bobbyholley) → review+
Comment on attachment 8844316 [details] [diff] [review]
Part 2: Make the non-virtual helpers for principal equality/subsumption checks inline

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

r=me assuming this is just moving code around (I didn't review in much detail).
Attachment #8844316 - Flags: review?(bobbyholley) → review+
Comment on attachment 8844317 [details] [diff] [review]
Part 3: Speed up principal access checks in WrapperFactory::Rewrap() by eliminating virtual dispatch and inling

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

Could be worth inlining these too at some point.
Attachment #8844317 - Flags: review?(bobbyholley) → review+

Comment 9

8 months ago
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c8caf7ff9f2
Part 1: Factor out more non-virtual helpers for principal equality/subsumption checks; r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/a84b225957b4
Part 2: Make the non-virtual helpers for principal equality/subsumption checks inline; r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9c886b3517f
Part 3: Speed up principal access checks in WrapperFactory::Rewrap() by eliminating virtual dispatch and inling; r=bholley

Comment 10

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2c8caf7ff9f2
https://hg.mozilla.org/mozilla-central/rev/a84b225957b4
https://hg.mozilla.org/mozilla-central/rev/a9c886b3517f
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.