Add resizable ArrayBuffers #646

bnoordhuis · 2024-11-04T22:50:17Z

This commit implements resizable ArrayBuffers - RABs for short - and extends typed arrays (TAs) to support fixed-length and length-tracking modes.

SharedArrayBuffers (SABs) also support the maxByteLength option now but I cheated and allocate all memory upfront because atomically resizing memory allocations is hard and this commit is already big and complex.

The lion's share is updating all the TA prototype methods to deal with RABs resizing underneath them. Method arguments can be arbitrary objects with arbitrary .valueOf methods and arbitrary side effects, like... resizing the RAB we're currently operating on.

This commit implements resizable ArrayBuffers - RABs for short - and extends typed arrays (TAs) to support fixed-length and length-tracking modes. SharedArrayBuffers (SABs) also support the maxByteLength option now but I cheated and allocate all memory upfront because atomically resizing memory allocations is hard and this commit is already big and complex. The lion's share is updating all the TA prototype methods to deal with RABs resizing underneath them. Method arguments can be arbitrary objects with arbitrary .valueOf methods and arbitrary side effects, like... resizing the RAB we're currently operating on.

bnoordhuis · 2024-11-04T22:56:52Z

quickjs.c

@@ -34116,6 +34130,7 @@ static int JS_WriteArrayBuffer(BCWriterState *s, JSValue obj)
    }
    bc_put_u8(s, BC_TAG_ARRAY_BUFFER);
    bc_put_leb128(s, abuf->byte_length);
+    bc_put_leb128(s, abuf->max_byte_length);


Another approach is to define a new tag BC_TAG_ARRAY_BUFFER_RESIZABLE and save a few bytes in the BC_TAG_ARRAY_BUFFER case (which is likely the common case)

Is saving a few bytes off the bc really critical?

I don't think so. I mentioned it for completeness.

bnoordhuis · 2024-11-04T23:03:08Z

quickjs.c

+        if (a_idx >= p->u.array.count || b_idx >= p->u.array.count)
+            return 0;


Interestingly, the spec also allows throwing an exception - and only here, in .sort() 🤷

bnoordhuis · 2024-11-04T23:06:21Z

test262_errors.txt

+test262/test/staging/ArrayBuffer/resizable/object-define-property-define-properties.js:55: strict mode: unexpected error: Test262Error: Expected a TypeError to be thrown but no exception was thrown at all
+test262/test/staging/ArrayBuffer/resizable/object-define-property-parameter-conversion-grows.js:67: strict mode: unexpected error: TypeError: out-of-bound index in typed array
+test262/test/staging/ArrayBuffer/resizable/object-define-property-parameter-conversion-shrinks.js:59: strict mode: unexpected error: Test262Error: Expected a TypeError to be thrown but no exception was thrown at all


Why are these failing? Honestly, because I didn't feel like fixing them anymore... the non-staging ones were already plenty of work, and these cases are even more deranged than usual.

saghul

Left a few questions / comments, impressive work!

Just to satisfy my curiosity: is this approach (realloc) better than mmap here? Wouldn't having the pointer be stable simplify things? I suppose platform differences are annoying here?

Also, fun fact: I thought of going for RAB at some point. My plan was to expose the allocation functions like one currently can for SAB, with the limitation that pointers need to be stable, and have a default which allocated everything upfront. Then maybe look at mmap :-P

saghul · 2024-11-05T08:17:43Z

quickjs.c

 static uint32_t typed_array_get_length(JSContext *ctx, JSObject *p);
 static JSValue JS_ThrowTypeErrorDetachedArrayBuffer(JSContext *ctx);
+// if you think the current name is lousy,
+// I considered naming it JS_ThrowTypeErrorRABOOB


Name looks good, you can drop the comment 😅

saghul · 2024-11-05T08:18:17Z

quickjs.c

                    if (!(flags & JS_PROP_DEFINE_PROPERTY))
                        return TRUE; // per spec: no OOB exception
+                // XXX(bnoordhuis) questionable but generic methods like


Ah lol, this conflicts with the PR I just made...

saghul · 2024-11-05T08:18:54Z

quickjs.c

@@ -34116,6 +34130,7 @@ static int JS_WriteArrayBuffer(BCWriterState *s, JSValue obj)
    }
    bc_put_u8(s, BC_TAG_ARRAY_BUFFER);
    bc_put_leb128(s, abuf->byte_length);
+    bc_put_leb128(s, abuf->max_byte_length);


Is saving a few bytes off the bc really critical?

saghul · 2024-11-05T08:25:13Z

quickjs.c

+    JS_FreeValue(ctx, obj);
+    if (JS_IsException(val))
+        return JS_EXCEPTION;
+    if (JS_IsUndefined(val))


What about null?

Allowed, curiously enough, and coerced to zero.

saghul · 2024-11-05T08:27:13Z

quickjs.c

+            max_len = abuf->max_byte_length;
+            if (new_len > max_len)
+                return JS_ThrowTypeError(ctx, "invalid array buffer length");
+            // TODO(bnoordhuis) support externally managed RABs


By externally managed you mean when a C API where the user can set allocation functions?

Yes. Buffers allocated by C code, not JS code.

saghul · 2024-11-05T08:28:05Z

quickjs.c

+        return JS_EXCEPTION;
+    if (abuf->shared) {
+        if (class_id == JS_CLASS_ARRAY_BUFFER)
+            return JS_ThrowTypeError(ctx, "resize called on SharedArrayBuffer");


When can this happen?

ArrayBuffer.prototype.resize.call(sab, 42) - but now that I think about it, that's already enforced by JS_GetOpaque2. I'll remove this.

saghul · 2024-11-05T08:28:13Z

quickjs.c

+            return JS_ThrowTypeError(ctx, "resize called on SharedArrayBuffer");
+    } else {
+        if (class_id == JS_CLASS_SHARED_ARRAY_BUFFER)
+            return JS_ThrowTypeError(ctx, "grow called on ArrayBuffer");


When can this happen?

saghul · 2024-11-05T08:32:45Z

quickjs.c

+    if (abuf->shared) {
+        if (len < abuf->byte_length)
+            goto bad_length;
+        // Note this is off-spec; there's supposed to be a single atomic


Out of curiosity: how is this supposed to work? As in, how would t1 let t0 know that they grew the SAB? Some kind of shared memory region where the length is stored or?

Yes, shared metadata.

saghul · 2024-11-05T08:40:06Z

quickjs.c

    if (JS_IsException(ta_buffer))
        goto exception;
    args[0] = this_val;
    args[1] = ta_buffer;
    args[2] = js_int32(offset);
    args[3] = js_int32(count);
+    // result is length-tracking if source TA is and no explicit count is given
+    if (ta->track_rab && JS_IsUndefined(argv[1]))


Should we check for isnull here too?

No; it mirrors the if (!JS_IsUndefined(argv[1])) guard about 30 lines up.

saghul · 2024-11-05T08:41:10Z

quickjs.c

+        JS_ThrowTypeErrorArrayBufferOOB(ctx);
+        return JS_EXCEPTION;


Suggested change

JS_ThrowTypeErrorArrayBufferOOB(ctx);

return JS_EXCEPTION;

return JS_ThrowTypeErrorArrayBufferOOB(ctx);

bnoordhuis

is this approach (realloc) better than mmap here? Wouldn't having the pointer be stable simplify things? I suppose platform differences are annoying here?

mmap's page granularity would be very wasteful for small arraybuffers. Even for larger arraybuffers you still end up wasting, on average, half a page.

Stable pointers could be very useful for SABs but for RABs I don't think it matters. TA methods are agnostic to where the data lives. The hard part is getting the bound checks right.

bnoordhuis · 2024-11-05T18:47:46Z

quickjs.c

@@ -34116,6 +34130,7 @@ static int JS_WriteArrayBuffer(BCWriterState *s, JSValue obj)
    }
    bc_put_u8(s, BC_TAG_ARRAY_BUFFER);
    bc_put_leb128(s, abuf->byte_length);
+    bc_put_leb128(s, abuf->max_byte_length);


I don't think so. I mentioned it for completeness.

bnoordhuis · 2024-11-05T18:49:20Z

quickjs.c

+    JS_FreeValue(ctx, obj);
+    if (JS_IsException(val))
+        return JS_EXCEPTION;
+    if (JS_IsUndefined(val))


Allowed, curiously enough, and coerced to zero.

bnoordhuis · 2024-11-05T18:49:57Z

quickjs.c

+            max_len = abuf->max_byte_length;
+            if (new_len > max_len)
+                return JS_ThrowTypeError(ctx, "invalid array buffer length");
+            // TODO(bnoordhuis) support externally managed RABs


Yes. Buffers allocated by C code, not JS code.

bnoordhuis · 2024-11-05T18:54:54Z

quickjs.c

+        return JS_EXCEPTION;
+    if (abuf->shared) {
+        if (class_id == JS_CLASS_ARRAY_BUFFER)
+            return JS_ThrowTypeError(ctx, "resize called on SharedArrayBuffer");


ArrayBuffer.prototype.resize.call(sab, 42) - but now that I think about it, that's already enforced by JS_GetOpaque2. I'll remove this.

bnoordhuis · 2024-11-05T18:55:28Z

quickjs.c

+    if (abuf->shared) {
+        if (len < abuf->byte_length)
+            goto bad_length;
+        // Note this is off-spec; there's supposed to be a single atomic


Yes, shared metadata.

bnoordhuis · 2024-11-05T18:57:02Z

quickjs.c

    if (JS_IsException(ta_buffer))
        goto exception;
    args[0] = this_val;
    args[1] = ta_buffer;
    args[2] = js_int32(offset);
    args[3] = js_int32(count);
+    // result is length-tracking if source TA is and no explicit count is given
+    if (ta->track_rab && JS_IsUndefined(argv[1]))


No; it mirrors the if (!JS_IsUndefined(argv[1])) guard about 30 lines up.

bnoordhuis added 2 commits November 4, 2024 23:49

fixup! SAB ser(des)

8d3babe

bnoordhuis commented Nov 4, 2024

View reviewed changes

fixup! initialize var

611b1ab

bnoordhuis mentioned this pull request Nov 5, 2024

Don't throw oob exception when setting numeric indexes on TAs #647

Merged

saghul approved these changes Nov 5, 2024

View reviewed changes

bnoordhuis commented Nov 5, 2024

View reviewed changes

bnoordhuis added 2 commits November 5, 2024 21:33

squash! review feedback

ab40bdb

squash! fixups

fda205a

bnoordhuis merged commit 37fe427 into quickjs-ng:master Nov 5, 2024
47 checks passed

bnoordhuis deleted the rab branch November 5, 2024 20:55

bnoordhuis mentioned this pull request Nov 5, 2024

Implement ResizableArrayBuffer [wip] #85

Closed

saghul mentioned this pull request Nov 7, 2024

"TypeError: out-of-bound numeric index" on typed arrays in strict mode #645

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add resizable ArrayBuffers #646

Add resizable ArrayBuffers #646

bnoordhuis commented Nov 4, 2024

bnoordhuis Nov 4, 2024

saghul Nov 5, 2024

bnoordhuis Nov 5, 2024

bnoordhuis Nov 4, 2024

bnoordhuis Nov 4, 2024

saghul left a comment

saghul Nov 5, 2024

saghul Nov 5, 2024

saghul Nov 5, 2024

saghul Nov 5, 2024

bnoordhuis Nov 5, 2024

saghul Nov 5, 2024

bnoordhuis Nov 5, 2024

saghul Nov 5, 2024

bnoordhuis Nov 5, 2024

saghul Nov 5, 2024

saghul Nov 5, 2024

bnoordhuis Nov 5, 2024

saghul Nov 5, 2024

bnoordhuis Nov 5, 2024

saghul Nov 5, 2024

bnoordhuis left a comment

bnoordhuis Nov 5, 2024

bnoordhuis Nov 5, 2024

bnoordhuis Nov 5, 2024

bnoordhuis Nov 5, 2024

bnoordhuis Nov 5, 2024

bnoordhuis Nov 5, 2024

		if (a_idx >= p->u.array.count \|\| b_idx >= p->u.array.count)
		return 0;

	JS_ThrowTypeErrorArrayBufferOOB(ctx);
	return JS_EXCEPTION;
	return JS_ThrowTypeErrorArrayBufferOOB(ctx);

Add resizable ArrayBuffers #646

Add resizable ArrayBuffers #646

Conversation

bnoordhuis commented Nov 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saghul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment