Closed Bug 773519 Opened 12 years ago Closed 12 years ago

Speed up string arguments by faking an XPCOM string

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 772466
Assignee: nobody → bzbarsky
Whiteboard: [need review]
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?
Attachment #641691 - Attachment is obsolete: true
Attachment #641691 - Flags: review?(peterv)
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+
> 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?).
Sounds good.
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.
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
Depends on: 1427028
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: