Closed
Bug 1344974
Opened 7 years ago
Closed 7 years ago
Eliminate virtual calls in xpc::AccessCheck methods
Categories
(Core :: XPConnect, enhancement)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
References
Details
Attachments
(3 files)
5.21 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
13.78 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
2.62 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
Follow-up from bug 1340710. Bobby, is this what you meant in bug 1340710 comment 77?
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(bobbyholley)
Assignee | ||
Comment 1•7 years 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.
Comment 2•7 years ago
|
||
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•7 years ago
|
Assignee: nobody → ehsan
Assignee | ||
Comment 3•7 years ago
|
||
Attachment #8844315 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8844316 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 5•7 years ago
|
||
Attachment #8844317 -
Flags: review?(bobbyholley)
Comment 6•7 years ago
|
||
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 7•7 years ago
|
||
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 8•7 years ago
|
||
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+
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•7 years 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
Closed: 7 years 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.
Description
•