Closed Bug 1339251 Opened 7 years ago Closed 7 years ago

Make Equals/Subsumes faster when comparing same objects

Categories

(Core :: Security, defect)

50 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(1 file, 3 obsolete files)

patch coming a bit later
Attached patch v1 (obsolete) — Splinter Review
bholley wanted to add something to BasePrincipal too, but since it inherits nsIPrincipal, and I couldn't really see very good use cases for those methods, and this is just very simple, here is a patch which cuts out pretty much all of principal handling from bug 1338802.
Summary: Inline some methods in principals → Make Equals/Subsumes faster when comparing same objects
Comment on attachment 8836956 [details] [diff] [review]
v1

I couldn't quite figure out useful inlining for BasePrincipal, since
Equals and Subsumes are the methods people are using.
Attachment #8836956 - Flags: review?(bobbyholley)
Comment on attachment 8836956 [details] [diff] [review]
v1

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

Generally fine, but I want to see the updated version because C++-in-xpidl is nasty and easy to mess up.

::: caps/nsIPrincipal.idl
@@ +46,5 @@
> +#ifdef DEBUG
> +      bool eq = false;
> +      MOZ_ASSERT_IF(this == aOther,
> +                    NS_SUCCEEDED(Equals(aOther, &eq)) && eq);
> +#endif

Please add a macro to do this, something like VERIFY_EQUALITY_FASTPATH. Also please add documentation around that explaining that this is an inline fast path to avoid virtual overhead etc when the principals are pointer-equal.

It would be nice to also document which situations actually lead to pointer-equal principals. Presumably google suite is triggering one of the inherit cases?
Attachment #8836956 - Flags: review?(bobbyholley) → review-
Comment on attachment 8839417 [details] [diff] [review]
v2

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

::: caps/nsIPrincipal.idl
@@ +42,5 @@
> +    return                                                     \
> +      this == aOther ||                                        \
> +      (NS_SUCCEEDED(method_(aOther, &retVal)) && retVal);      \
> +  }
> +#endif

Why do we need two copies of this method? Shouldn't the first compile to the second in opt builds?

If the issue is the unused variable, we can just use mozilla::unused, or DebugOnly, or cast to void.
Attachment #8839417 - Flags: review?(bobbyholley) → review-
I have no idea what your comment means.
Flags: needinfo?(bobbyholley)
Attached patch v3 (obsolete) — Splinter Review
This hopefully compiles
Attachment #8839417 - Attachment is obsolete: true
Flags: needinfo?(bobbyholley)
Attachment #8839691 - Flags: review?(bobbyholley)
Comment on attachment 8839691 [details] [diff] [review]
v3

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

r=me with that.

::: caps/nsIPrincipal.idl
@@ +23,5 @@
> +
> +#define DECL_FAST_INLINE_HELPER(method_)                       \
> +  inline bool method_(nsIPrincipal* aOther)                    \
> +  {                                                            \
> +                                                               \

Nit: unnecessary newline.

@@ +117,5 @@
>  
>      %{C++
> +      DECL_FAST_INLINE_HELPER(Subsumes)
> +      DECL_FAST_INLINE_HELPER(SubsumesConsideringDomain)
> +      DECL_FAST_INLINE_HELPER(SubsumesConsideringDomainIgnoringFPD)

We should undef the macro here.
Attachment #8839691 - Flags: review?(bobbyholley) → review+
Attached patch v4Splinter Review
Attachment #8839691 - Attachment is obsolete: true
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/acdb8e6dc477
Make Equals/Subsumes faster when comparing same objects, r=bholley
https://hg.mozilla.org/mozilla-central/rev/acdb8e6dc477
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.