Closed Bug 559476 Opened 11 years ago Closed 11 years ago

Do a fast copy of dense primitive arrays when making a structured clone

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

References

Details

Attachments

(1 file, 5 obsolete files)

Attached patch Patch, v1 (obsolete) — Splinter Review
For big data arrays it would be better to quickly copy rather than enumerate item by item. This only works if the array is dense and is made up of primitives. As soon as we have an object in the array we have to bail and enumerate anyway.
Attachment #439143 - Flags: review?(vladimir)
Attachment #439143 - Flags: review?(jst)
Attached patch Patch, v1.1 (obsolete) — Splinter Review
This removes the memcopy in favor of walking the array only once.
Attachment #439143 - Attachment is obsolete: true
Attachment #439400 - Flags: review?(vladimir)
Attachment #439400 - Flags: review?(jst)
Attachment #439143 - Flags: review?(vladimir)
Attachment #439143 - Flags: review?(jst)
Attachment #439400 - Flags: review?(jst) → review+
Attachment #439400 - Flags: review?(vladimir) → review?(jorendorff)
Attached patch Patch, v2 (obsolete) — Splinter Review
Updated with fixes suggested by jorendorff and vlad.
Attachment #439400 - Attachment is obsolete: true
Attachment #440074 - Flags: review?(mrbkap)
Attachment #439400 - Flags: review?(jorendorff)
Attached patch Patch, v3 (obsolete) — Splinter Review
Updated with comments from mrbkap.
Attachment #440074 - Attachment is obsolete: true
Attachment #440424 - Flags: review?(mrbkap)
Attachment #440074 - Flags: review?(mrbkap)
Attached patch Patch, v3.1 (obsolete) — Splinter Review
This is nearly the same but uses memcpy instead of walking the array twice.
Attachment #440424 - Attachment is obsolete: true
Attachment #440503 - Flags: review?(mrbkap)
Attachment #440424 - Flags: review?(mrbkap)
Comment on attachment 440503 [details] [diff] [review]
Patch, v3.1

I talked with bent on IRC about this patch. He has a new patch coming.
Attachment #440503 - Flags: review?(mrbkap)
Attached patch Patch, v3.2Splinter Review
One more
Attachment #440503 - Attachment is obsolete: true
Attachment #440640 - Flags: review?(mrbkap)
Comment on attachment 440640 [details] [diff] [review]
Patch, v3.2

Great, thanks.
Attachment #440640 - Flags: review?(mrbkap) → review+
Comment on attachment 440640 [details] [diff] [review]
Patch, v3.2

>+  JSObject* newArray;
>+  if (js_CloneDensePrimitiveArray(cx, obj, &newArray) && newArray) {
>+    return SetPropertyOnValueOrObject(cx, OBJECT_TO_JSVAL(newArray), rval, robj,
>+                                      rid);
>+  }

I realized after I marked this: if js_CloneDensePrimitiveArray returns false, then you need to return failure, otherwise you could leave an exception unreported.
Ah true. I'll fix before checkin.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 582451
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.