Closed
Bug 834877
Opened 11 years ago
Closed 11 years ago
Introduce a cheaper way to do string return values in common cases
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(5 files, 5 obsolete files)
5.13 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
2.93 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
9.88 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
7.73 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
3.25 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
Specifically, any time the callee is returning a stringbuffer that they plan to hold on to as well. Doing this for now because I have no faith in us ever fixing stringbuffer/string refcounting/slowness.
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Comment 2•11 years ago
|
||
Assignee | ||
Comment 3•11 years ago
|
||
Assignee | ||
Comment 4•11 years ago
|
||
Assignee | ||
Comment 5•11 years ago
|
||
Assignee | ||
Comment 6•11 years ago
|
||
Those patches still need a bunch of cleanup, but some preliminary numbers are at http://dromaeo.com/?id=189050,189051,189052,189053 First column is without patch, second is with patch, third is Chrome dev, fourth is WebKit nightly. Note the getAttribute and element.property numbers.
Assignee | ||
Comment 7•11 years ago
|
||
So I'm not entirely convinced by all the inlining happening here, in terms of codesize. I measured what happens to .id numbers in a test like the one in Dromaeo's dom-attr if I undo some of it. All number below are in ns. For scale, the time with all the inlining on my machine is about 16ns, the time on trunk is about 35ns, WebKit nightly is about 18ns (but note that they have separate storage for the id specifically, so it's a bit of a special-case there). Cost of not inlining NonVoidStringToJsval: 1.5ns Cost of then not inlining StringBufferToJSVal: 1-1.5ns Cost of not inlining nsAttrValue::ToString: 1.5-2ns Cost of not inlining Element::GetAttr: 1.5-2ns The total cost of uninlining everything is about 6.5ns, so lands us in the 22.5ns range. Still much better than where we are now. ;) Through all this, the getAttribute numbers I'm measuring are pretty stable. It's about 52ns without these patches, about 36-37ns with the patches no matter what happens with the inlining. Peter, any thoughts?
Assignee | ||
Comment 8•11 years ago
|
||
Attachment #707022 -
Flags: review?(peterv)
Assignee | ||
Updated•11 years ago
|
Attachment #706732 -
Attachment is obsolete: true
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #707023 -
Flags: review?(peterv)
Assignee | ||
Updated•11 years ago
|
Attachment #706733 -
Attachment is obsolete: true
Assignee | ||
Comment 10•11 years ago
|
||
Attachment #707024 -
Flags: review?(peterv)
Assignee | ||
Updated•11 years ago
|
Attachment #706734 -
Attachment is obsolete: true
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #707025 -
Flags: review?(peterv)
Assignee | ||
Updated•11 years ago
|
Attachment #706735 -
Attachment is obsolete: true
Assignee | ||
Comment 12•11 years ago
|
||
Attachment #707026 -
Flags: review?(peterv)
Assignee | ||
Updated•11 years ago
|
Attachment #706736 -
Attachment is obsolete: true
Comment 13•11 years ago
|
||
>+ * The proper way to store a value in this class is to either to nothing (which
"to nothing"?
Assignee | ||
Comment 14•11 years ago
|
||
> "to nothing"?
"to do nothing". Will fix.
Assignee | ||
Updated•11 years ago
|
Whiteboard: [need review]
Comment 15•11 years ago
|
||
Comment on attachment 707022 [details] [diff] [review] part 1. Add a DOMString struct to use for binding return values. Review of attachment 707022 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/BindingDeclarations.h @@ +90,5 @@ > + MOZ_ASSERT(!mIsNull, "Caller should have checked IsNull() first"); > + return mString.empty(); > + } > + > + // Get the stringbuffer. This can only be called if HasStringBuffer() and I'd say "if HasStringBuffer() returned true"
Attachment #707022 -
Flags: review?(peterv) → review+
Updated•11 years ago
|
Attachment #707023 -
Flags: review?(peterv) → review+
Comment 16•11 years ago
|
||
Comment on attachment 707024 [details] [diff] [review] part 3. Add faster DOMString-to-JS conversion code. Review of attachment 707024 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/XPCString.cpp @@ +84,5 @@ > + jschar *chars = reinterpret_cast<jschar *> > + (JS_malloc(cx, (length + 1) * > + sizeof(jschar))); > + if (!chars) > + return JSVAL_NULL; While you're here make this JS::NullValue() too. @@ +90,5 @@ > + if (length && !CopyUnicodeTo(readable, 0, > + reinterpret_cast<PRUnichar *>(chars), > + length)) { > + JS_free(cx, chars); > + return JSVAL_NULL; And here. ::: js/xpconnect/src/xpcpublic.h @@ +299,5 @@ > + } > + > + nsStringBuffer* buf = str.StringBuffer(); > + bool shared; > + if (!XPCStringConvert::StringBufferToJSVal(cx, str.StringBuffer(), length, Pass in buf here.
Attachment #707024 -
Flags: review?(peterv) → review+
Updated•11 years ago
|
Attachment #707025 -
Flags: review?(peterv) → review+
Updated•11 years ago
|
Attachment #707026 -
Flags: review?(peterv) → review+
Assignee | ||
Comment 17•11 years ago
|
||
Addressed those comments on part 3.
Assignee | ||
Comment 18•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a158459ea7d https://hg.mozilla.org/integration/mozilla-inbound/rev/c4209fe94d30 https://hg.mozilla.org/integration/mozilla-inbound/rev/9ed387b675bb https://hg.mozilla.org/integration/mozilla-inbound/rev/4f59efe77333 https://hg.mozilla.org/integration/mozilla-inbound/rev/27e211d02a7a and documented at https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#DOMString and https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#DOMString-helper
Flags: in-testsuite+
Keywords: dev-doc-complete
Whiteboard: [need review]
Target Milestone: --- → mozilla21
Assignee | ||
Comment 19•11 years ago
|
||
> "to do nothing". Will fix. And of course I failed to do that... Fixed in https://hg.mozilla.org/integration/mozilla-inbound/rev/361b83661de1
Comment 20•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6a158459ea7d https://hg.mozilla.org/mozilla-central/rev/c4209fe94d30 https://hg.mozilla.org/mozilla-central/rev/9ed387b675bb https://hg.mozilla.org/mozilla-central/rev/4f59efe77333 https://hg.mozilla.org/mozilla-central/rev/27e211d02a7a https://hg.mozilla.org/mozilla-central/rev/361b83661de1
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•