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)

x86
macOS
defect
Not set
normal

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.
Blocks: 811832
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.
Blocks: 772466
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?
Attachment #706732 - Attachment is obsolete: true
Attachment #706733 - Attachment is obsolete: true
Attachment #706734 - Attachment is obsolete: true
Attachment #706735 - Attachment is obsolete: true
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.
Blocks: 835800
> "to do nothing".  Will fix.

And of course I failed to do that...  Fixed in https://hg.mozilla.org/integration/mozilla-inbound/rev/361b83661de1
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: