Closed Bug 934817 Opened 6 years ago Closed 6 years ago

JS_NewUCStringCopyN should do the usual 0-length optimization


(Core :: JavaScript Engine, defect)

Not set





(Reporter: bzbarsky, Assigned: mz_mhs-ctb)



(3 files, 4 obsolete files)

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?
Assignee: nobody → mz_mhs-ctb
OS: Mac OS X → All
Hardware: x86 → All
For this, would it be possible to use cx->runtime()->emptyString like JS_NewUCStringCopyZ does?
Flags: needinfo?(bzbarsky)
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.)
Flags: needinfo?(bzbarsky)
Attached patch 0-length optimization (obsolete) — Splinter Review
Attachment #8375245 - Flags: review?(luke)
Would be nice to clean up XPCStringConvert::ReadableToJSVal too.
Attachment #8375245 - Flags: review?(luke) → review+
Do we also want to optimize JS_NewExternalString?
Attached patch Remove 0-length check (obsolete) — Splinter Review
Attachment #8375287 - Flags: review?(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.
Attachment #8375287 - Flags: review?(bzbarsky) → review+
• Can we also remove similar JS_GetEmptyStringValue fast paths from BindingUtils.cpp and XPCConvert.cpp?

• 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).
Start baking on m-i/m-c.
Attachment #8375245 - Attachment is obsolete: true
Attachment #8375942 - Flags: review+
Attachment #8375942 - Flags: checkin?
Attached patch Remove XPConnect fast-paths. (obsolete) — Splinter Review
Attachment #8375287 - Attachment is obsolete: true
Attachment #8375943 - Flags: review?(bzbarsky)
Attachment #8375946 - Flags: review?(luke)
Attachment #8375946 - Flags: review?(luke) → review?(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.
Attachment #8375946 - Flags: review?(bzbarsky) → review+
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.
Attachment #8375943 - Flags: review?(bzbarsky) → review+
Attachment #8375942 - Flags: checkin?
I'm not sure this is worth uplifting to 29.
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.