1339251 - Make Equals/Subsumes faster when comparing same objects

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

patch coming a bit later

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v1

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.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c350e16a3211206755fec266480df672fdca738

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 4

[Bobby Holley (:bholley)]

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?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2

https://treeherder.mozilla.org/#/jobs?repo=try&revision=37770c8471a9104cd03cc1d80ed08184d85fad2c

Comment 6

[Bobby Holley (:bholley)]

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.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

I have no idea what your comment means.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v3

This hopefully compiles

Comment 9

[Bobby Holley (:bholley)]

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.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v4

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/acdb8e6dc477
Make Equals/Subsumes faster when comparing same objects, r=bholley

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/acdb8e6dc477