773519 - Speed up string arguments by faking an XPCOM string

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

This is totally evil, but the XPCOM string constructors/destructors are pain and suffering perf-wise.

The patch coming up gets the overhead per string argument down from 15ns to what looks like 4ns on my hardware.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Speed up string argument conversion in DOM bindings by faking an XPCOM string.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Speed up string argument conversion in DOM bindings by faking an XPCOM string.

Comment 3

[Peter Van der Beken [:peterv]]

Comment on attachment 641691 [details] [diff] [review]
Speed up string argument conversion in DOM bindings by faking an XPCOM string.

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

::: dom/bindings/BindingUtils.h
@@ +752,5 @@
> +    MOZ_STATIC_ASSERT(sizeof(FakeDependentString) == sizeof(nsDependentString),
> +                      "Must have right object size");
> +    // XXXbz would be nice to assert things about member order too,
> +    // but those are protected on nsDependentString.
> +  }

Could we add an inner class that inherits from nsDependentString so we have access to the members on nsDependentString and can statically assert things about them?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Excellent idea. With that inner class, asserting the object layout now.

Comment 5

[Peter Van der Beken [:peterv]]

Comment on attachment 642679 [details] [diff] [review]
Excellent idea.  With that inner class, asserting the object layout now.

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

::: dom/bindings/BindingUtils.h
@@ +760,5 @@
> +    mLength = aLength;
> +  }
> +
> +  void SetIsVoid(bool aSetVoid) {
> +    // aSetVoid true means voided, false means empty string.

I think we should be compatible with the nsAString stuff (false just unsets the flag, true sets the flag and truncates).

Comment 6

[Boris Zbarsky [:bzbarsky]]

> I think we should be compatible with the nsAString stuff 

The thing is, that requires me to set mData and mLength in the constructor, only to have it clobbered by the Rebind() in the common case...

I guess I can do that if you feel pretty strongly about it.  Alternately, I could change the naming of SetIsVoid (and of Rebind if needed): the only caller is ConvertJSValueToString and we can just change the contract of its interaction with the string.  In particular, we could just have a SetNull() with no argument for the null case, and a SetEmpty() for the empty case.

Comment 7

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky (:bz) from comment #6)
> In particular, we could just have a SetNull()
> with no argument for the null case, and a SetEmpty() for the empty case.

Let's do this (though maybe Truncate instead of SetEmpty?).

Comment 8

[Boris Zbarsky [:bzbarsky]]

Sounds good.

Comment 9

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9deb8edb5070

Comment 10

[Ed Morley [:emorley]]

Sorry, push backed out for compilation errors on all platforms:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=9deb8edb5070

eg: https://tbpl.mozilla.org/php/getParsedLog.php?id=13605808&tree=Mozilla-Inbound
{
In file included from ../../../../js/xpconnect/wrappers/XrayWrapper.cpp:22:0:
../../../dist/include/nsTSubstring.h:639:18: error: 'nsAString_internal::char_type* nsAString_internal::mData' is protected
../../../dist/include/mozilla/dom/BindingUtils.h:794:77: error: within this context
../../../dist/include/nsTSubstring.h:640:17: error: 'nsAString_internal::size_type nsAString_internal::mLength' is protected
../../../dist/include/mozilla/dom/BindingUtils.h:795:79: error: within this context
../../../dist/include/nsTSubstring.h:641:16: error: 'PRUint32 nsAString_internal::mFlags' is protected
../../../dist/include/mozilla/dom/BindingUtils.h:796:78: error: within this context
make[6]: *** [XrayWrapper.o] Error 1
}

https://hg.mozilla.org/integration/mozilla-inbound/rev/8212be806c67

Comment 11

[[e-mail]]

Comment on attachment 642679 [details] [diff] [review]
Excellent idea.  With that inner class, asserting the object layout now.

>+  void SetIsVoid(bool aSetVoid) {
>+    // aSetVoid true means voided, false means empty string.
>+    mData = nsnull;
This is not a good idea; BeginReading()[0] is still supported although not many people do it.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Relanded with an attempt to make the static asserts compile on gcc and maybe msvc, not just clang: https://hg.mozilla.org/integration/mozilla-inbound/rev/0e6c79d17f42

> This is not a good idea;

Hrm.  Replaced with nsDependentString::char_traits::sEmptyBuffer :

  https://hg.mozilla.org/integration/mozilla-inbound/rev/2b31d5caf239

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0e6c79d17f42
https://hg.mozilla.org/mozilla-central/rev/2b31d5caf239