Introduce a cheaper way to do string return values in common cases

RESOLVED FIXED in mozilla21

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla21
x86
Mac OS X
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 5 obsolete attachments)

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.
Created attachment 706732 [details] [diff] [review]
part 1.  Add a DOMString struct to use for binding return values.
Created attachment 706733 [details] [diff] [review]
part 2.  Start using DOMString as the return value for strings.
Created attachment 706734 [details] [diff] [review]
part 3.  Add faster DOMString-to-JS conversion code.
Created attachment 706735 [details] [diff] [review]
part 4.  Add overloads of GetAttr() and GetId() that take a DOMString.
Created attachment 706736 [details] [diff] [review]
part 5.  Add an overload of GetAttribute that takes a DOMString.
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.
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?
Created attachment 707022 [details] [diff] [review]
part 1.  Add a DOMString struct to use for binding return values.
Attachment #707022 - Flags: review?(peterv)
Attachment #706732 - Attachment is obsolete: true
Created attachment 707023 [details] [diff] [review]
part 2.  Start using DOMString as the return value for strings.
Attachment #707023 - Flags: review?(peterv)
Attachment #706733 - Attachment is obsolete: true
Created attachment 707024 [details] [diff] [review]
part 3.  Add faster DOMString-to-JS conversion code.
Attachment #707024 - Flags: review?(peterv)
Attachment #706734 - Attachment is obsolete: true
Created attachment 707025 [details] [diff] [review]
part 4.  Add overloads of GetAttr() and GetId() that take a DOMString.
Attachment #707025 - Flags: review?(peterv)
Attachment #706735 - Attachment is obsolete: true
Created attachment 707026 [details] [diff] [review]
part 5.  Add an overload of GetAttribute that takes a DOMString.
Attachment #707026 - Flags: review?(peterv)
Attachment #706736 - Attachment is obsolete: true
>+ * The proper way to store a value in this class is to either to nothing (which

"to nothing"?
> "to nothing"?

"to do nothing".  Will fix.
Whiteboard: [need review]
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+
Attachment #707023 - Flags: review?(peterv) → review+
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+
Attachment #707025 - Flags: review?(peterv) → review+
Attachment #707026 - Flags: review?(peterv) → review+
Addressed those comments on part 3.
> "to do nothing".  Will fix.

And of course I failed to do that...  Fixed in https://hg.mozilla.org/integration/mozilla-inbound/rev/361b83661de1
You need to log in before you can comment on or make changes to this bug.