Speed up string arguments by faking an XPCOM string

RESOLVED FIXED in mozilla17

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug)

unspecified
mozilla17
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 772466
(Assignee)

Comment 1

5 years ago
Created attachment 641683 [details] [diff] [review]
Speed up string argument conversion in DOM bindings by faking an XPCOM string.
Attachment #641683 - Flags: review?(peterv)
(Assignee)

Updated

5 years ago
Assignee: nobody → bzbarsky
Whiteboard: [need review]
(Assignee)

Comment 2

5 years ago
Created attachment 641691 [details] [diff] [review]
Speed up string argument conversion in DOM bindings by faking an XPCOM string.
Attachment #641691 - Flags: review?(peterv)
(Assignee)

Updated

5 years ago
Attachment #641683 - Attachment is obsolete: true
Attachment #641683 - Flags: review?(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?
(Assignee)

Comment 4

5 years ago
Created attachment 642679 [details] [diff] [review]
Excellent idea.  With that inner class, asserting the object layout now.
(Assignee)

Updated

5 years ago
Attachment #641691 - Attachment is obsolete: true
Attachment #641691 - Flags: review?(peterv)
(Assignee)

Updated

5 years ago
Attachment #642679 - Flags: review?(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).
Attachment #642679 - Flags: review?(peterv) → review+
(Assignee)

Comment 6

5 years ago
> 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.
(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?).
(Assignee)

Comment 8

5 years ago
Sounds good.
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9deb8edb5070
Flags: in-testsuite-
Whiteboard: [need review]
Target Milestone: --- → mozilla17
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
Target Milestone: mozilla17 → ---
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.
(Assignee)

Comment 12

5 years ago
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
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/0e6c79d17f42
https://hg.mozilla.org/mozilla-central/rev/2b31d5caf239
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.