Skip to content

Implement ResizableArrayBuffer [wip] #85

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from

Conversation

littledivy
Copy link
Contributor

https://github.com/tc39/proposal-resizablearraybuffer

Changes:

  • Add max_byte_length and resizable fields to JSArrayBuffer
  • Add is_length_tracking and is_backed_by_rab fields to JSTypedArray
  • Add js_array_buffer_constructor4 for constructing RAB with maxByteLength
  • Implement resize() on top of js_realloc
  • Handle variable length tracking for JSTypedArray

Did not implement SharedArrayBuffer in this patch.

@bnoordhuis
Copy link
Contributor

Run build/run-test262 -c test.conf -a -u to update test262_errors.txt.

quickjs.c Outdated
Comment on lines 47402 to 47411
val = JS_GetProperty(ctx, val, JS_ATOM_maxByteLength);
if (!JS_IsUndefined(val)) {
if (JS_IsException(val) || JS_ToIndex(ctx, &max_len, val)) {
JS_FreeValue(ctx, val);
return JS_EXCEPTION;
}
return js_array_buffer_constructor4(ctx, JS_UNDEFINED, len, max_len, TRUE,
JS_CLASS_ARRAY_BUFFER, NULL, js_array_buffer_free, NULL,
TRUE);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This leaks val (when it's not an int32), JS_ToIndex doesn't free it.

quickjs.c Outdated
Comment on lines 47493 to 47495
if (abuf->shared) {
return JS_ThrowTypeError(ctx, "resizable called on SharedArrayBuffer");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize the code isn't 100% consistent in this respect but let's drop the braces for single-line if statements.

quickjs.c Outdated
Comment on lines 47655 to 47658
if (abuf->detached) {
JS_ThrowTypeErrorDetachedArrayBuffer(ctx);
return JS_EXCEPTION;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be just:

Suggested change
if (abuf->detached) {
JS_ThrowTypeErrorDetachedArrayBuffer(ctx);
return JS_EXCEPTION;
}
if (abuf->detached)
return JS_ThrowTypeErrorDetachedArrayBuffer(ctx);

quickjs.c Outdated
JS_ThrowRangeError(ctx, "new length exceeds max length");
return JS_EXCEPTION;
}
abuf->data = js_realloc(ctx, abuf->data, new_len);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should handle reallocation failures. The new_len == 0 case is kind of tricky because the allocator can either return NULL or a unique pointer. Maybe easiest to js_free() when new_len == 0.

quickjs.c Outdated
/* Check if RAB has shrunk & start offset is OOB */
if (ta->offset > abuf->byte_length)
return JS_ThrowRangeError(ctx, "invalid typed array offset");
return JS_NewInt32(ctx, (abuf->byte_length - ta->offset));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return JS_NewInt32(ctx, (abuf->byte_length - ta->offset));
return JS_NewUint32(ctx, abuf->byte_length - ta->offset);

Because ta->offset is uint32_t. abuf->byte_length probably should be too but one thing at a time.

@littledivy
Copy link
Contributor Author

It doesn't look like we can use realloc for this:

Resiazable AB length can shrink mid iteration (eg: Array#every) and the tests expect the initial buffer elements to not be freed. So we want the backing buffer to not move and implement this by reserving pages and using them during resize (like V8)

@bnoordhuis
Copy link
Contributor

Resiazable AB length can shrink mid iteration (eg: Array#every) and the tests expect the initial buffer elements to not be freed. So we want the backing buffer to not move and implement this by reserving pages and using them during resize (like V8)

If you mean mmap'ing memory, I don't think we want to go there just yet. Can you point me to a test that triggers that behavior?

@littledivy
Copy link
Contributor Author

@littledivy
Copy link
Contributor Author

I guess I'll close this one for now until we wanna mmap

@bnoordhuis
Copy link
Contributor

For posterity: RABs landed just now via #646 - and without an mmap call in sight :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants