934817 - JS_NewUCStringCopyN should do the usual 0-length optimization

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Right now XPConnect code explicitly tests for length 0 and returns JS_GetEmptyStringValue in that case, but it seems like this would be better if it happened in JS_NewUCStringCopyN instead.  If we do that, we can remove the extra length == 0 check from XPCStringConvert::ReadableToJSVal.

Also, JS_NewStringCopyN can probably have the same thing going on, right?

Comment 1

[Michael]

For this, would it be possible to use cx->runtime()->emptyString like JS_NewUCStringCopyZ does?

Comment 2

[Jeff Walden [:Waldo]]

Sure.  Or, very very very slightly better, use cx->names().empty.  (Ideally cx->runtime()->emptyString will go away eventually, but there's some C++ nitpickiness to work through before that can happen, going from memory.)

Comment 3

[Michael]

Attachment: 0-length optimization

Comment 4

[Boris Zbarsky [:bzbarsky]]

Would be nice to clean up XPCStringConvert::ReadableToJSVal too.

Comment 5

[Michael]

Do we also want to optimize JS_NewExternalString?

Comment 6

[Michael]

Attachment: Remove 0-length check

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8375287 [details] [diff] [review]
Remove 0-length check

Hmm.  I'm not sure about JS_NewExternalString, honestly.

r=me on this, though.

Comment 8

[Chris Peterson [:cpeterson]]

• Can we also remove similar JS_GetEmptyStringValue fast paths from BindingUtils.cpp and XPCConvert.cpp?

https://mxr.mozilla.org/mozilla-central/source/dom/bindings/BindingUtils.cpp#2089
https://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCConvert.cpp#295

• Can we uplift the JS_New*StringCopyN optimizations to Aurora 29? These code changes are trivial and we have four weeks of bake time before Aurora 29 would be uplifted to Beta (March 17).

Comment 9

[Michael]

Attachment: Bug 934817 Part 1: JS_New*StringCopyN should do 0-length optimization. r=luke

Start baking on m-i/m-c.

Comment 10

[Michael]

Attachment: Remove XPConnect fast-paths.

Comment 11

[Michael]

Attachment: Remove dom/bindings/BindingUtils.cpp usage

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8375946 [details] [diff] [review]
Remove dom/bindings/BindingUtils.cpp usage

r=me if you fix the typo in the commit message and make it explain what usage you are removing.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8375943 [details] [diff] [review]
Remove XPConnect fast-paths.

r=me, but please fix the commit message to reflect what the patch is doing.

Comment 14

[Boris Zbarsky [:bzbarsky]]

I'm not sure this is worth uplifting to 29.

Comment 15

[Michael]

Attachment: Bug 934817 Part 2: Remove 0-len check in XPCStringConvert::ReadableToJSVal and XPCConvert::NativeData2JS. r=bz

Comment 16

[Michael]

Attachment: Bug 934817 Part 2.5: Remove 0-length string check in dom/bindings/BindingUtils.cpp. r=bz

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d63648c03b20
https://hg.mozilla.org/integration/mozilla-inbound/rev/f513ce12acde
https://hg.mozilla.org/integration/mozilla-inbound/rev/4081bcbf9285

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d63648c03b20
https://hg.mozilla.org/mozilla-central/rev/f513ce12acde
https://hg.mozilla.org/mozilla-central/rev/4081bcbf9285