Last Comment Bug 773519 - Speed up string arguments by faking an XPCOM string
: Speed up string arguments by faking an XPCOM string
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla17
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on:
Blocks: ParisBindings 772466
  Show dependency treegraph
 
Reported: 2012-07-12 18:28 PDT by Boris Zbarsky [:bz]
Modified: 2012-07-18 05:55 PDT (History)
4 users (show)
bzbarsky: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Speed up string argument conversion in DOM bindings by faking an XPCOM string. (4.91 KB, patch)
2012-07-12 18:32 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Speed up string argument conversion in DOM bindings by faking an XPCOM string. (5.02 KB, patch)
2012-07-12 19:00 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Excellent idea. With that inner class, asserting the object layout now. (5.85 KB, patch)
2012-07-16 12:26 PDT, Boris Zbarsky [:bz]
peterv: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2012-07-12 18:28:40 PDT
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 [:bz] 2012-07-12 18:32:32 PDT
Created attachment 641683 [details] [diff] [review]
Speed up string argument conversion in DOM bindings by faking an XPCOM string.
Comment 2 Boris Zbarsky [:bz] 2012-07-12 19:00:11 PDT
Created attachment 641691 [details] [diff] [review]
Speed up string argument conversion in DOM bindings by faking an XPCOM string.
Comment 3 Peter Van der Beken [:peterv] - away till Aug 1st 2012-07-16 10:42:02 PDT
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 [:bz] 2012-07-16 12:26:55 PDT
Created attachment 642679 [details] [diff] [review]
Excellent idea.  With that inner class, asserting the object layout now.
Comment 5 Peter Van der Beken [:peterv] - away till Aug 1st 2012-07-16 14:15:04 PDT
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 [:bz] 2012-07-16 14:24:03 PDT
> 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] - away till Aug 1st 2012-07-17 01:37:47 PDT
(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 [:bz] 2012-07-17 08:00:48 PDT
Sounds good.
Comment 10 Ed Morley [:emorley] 2012-07-17 09:50:47 PDT
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 neil@parkwaycc.co.uk 2012-07-17 09:53:20 PDT
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 [:bz] 2012-07-17 10:30:51 PDT
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

Note You need to log in before you can comment on or make changes to this bug.