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

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
8 years ago

People

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

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

Created attachment 439143 [details] [diff] [review]
Patch, v1

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)
Created attachment 439400 [details] [diff] [review]
Patch, v1.1

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)

Updated

8 years ago
Attachment #439400 - Flags: review?(jst) → review+
Attachment #439400 - Flags: review?(vladimir) → review?(jorendorff)
Created attachment 440074 [details] [diff] [review]
Patch, v2

Updated with fixes suggested by jorendorff and vlad.
Attachment #439400 - Attachment is obsolete: true
Attachment #440074 - Flags: review?(mrbkap)
Attachment #439400 - Flags: review?(jorendorff)
Created attachment 440424 [details] [diff] [review]
Patch, v3

Updated with comments from mrbkap.
Attachment #440074 - Attachment is obsolete: true
Attachment #440424 - Flags: review?(mrbkap)
Attachment #440074 - Flags: review?(mrbkap)
Created attachment 440503 [details] [diff] [review]
Patch, v3.1

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)
Created attachment 440640 [details] [diff] [review]
Patch, v3.2

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
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

8 years ago
Depends on: 582451
You need to log in before you can comment on or make changes to this bug.