Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign up[Do not merge] WIP: Accept typed array arguments in codegen #20205
Conversation
highfive
commented
Mar 5, 2018
|
Heads up! This PR modifies the following files:
|
| @@ -871,7 +872,44 @@ def wrapObjectTemplate(templateBody, nullValue, isDefinitelyObject, type, | |||
| return handleOptional(templateBody, declType, handleDefaultNull("None")) | |||
|
|
|||
| if type.isSpiderMonkeyInterface(): | |||
| raise TypeError("Can't handle SpiderMonkey interface arguments yet") | |||
This comment has been minimized.
This comment has been minimized.
Xanewok
Mar 5, 2018
Author
Contributor
This is really for typed arrays, whereas isSpiderMonkeyInterface() also returns true for a ReadableStream type - should I explicitly handle only typed arrays here for now?
This comment has been minimized.
This comment has been minimized.
| @@ -1245,6 +1284,7 @@ def __init__(self, argument, index, args, argc, descriptorProvider, | |||
| isClamp=argument.clamp, | |||
| isMember="Variadic" if argument.variadic else False, | |||
| isAutoRooted=type_needs_auto_root(argument.type), | |||
| needsRootingInto="arg_root" if type_conversion_needs_root(argument.type) else False, | |||
This comment has been minimized.
This comment has been minimized.
nox
Mar 6, 2018
Member
What is this new argument for? It seems weird to me that it is either "arg_root" or False.
This comment has been minimized.
This comment has been minimized.
Xanewok
Mar 6, 2018
Author
Contributor
The current typed array conversion API requires an existing stack root, so I wanted to also pass the name of stack root here or False if it's not needed.
In general I don't feel too good about it, since it's only used with argument conversion code and the arg_root name is duplicated anyway.
This seemed like a general mechanism that could be supported (e.g. union member conversions use a stack root before the conversion), but I think we can avoid introducing this by attempting the conversion on unrooted object and only root before the call (like with custom auto rooter), but this'd warrant changes to typed arrays API in rust-mozjs.
Should I do that?
| def type_conversion_needs_root(t): | ||
| assert isinstance(t, IDLObject), (t, type(t)) | ||
|
|
||
| if t.isType(): |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Xanewok
Mar 6, 2018
Author
Contributor
I don't think it should. To be quite frank, I blindly copy-pasted the checking logic from type_needs_tracing to be safe
| if t.isType(): | ||
| return t.isSpiderMonkeyInterface() | ||
|
|
||
| assert False, (t, type(t)) |
This comment has been minimized.
This comment has been minimized.
nox
Mar 6, 2018
Member
Please assert that t.isType() is True instead and unconditionally return t.isSpiderMonkeyInterface().
This comment has been minimized.
This comment has been minimized.
| @@ -871,7 +872,44 @@ def wrapObjectTemplate(templateBody, nullValue, isDefinitelyObject, type, | |||
| return handleOptional(templateBody, declType, handleDefaultNull("None")) | |||
|
|
|||
| if type.isSpiderMonkeyInterface(): | |||
| raise TypeError("Can't handle SpiderMonkey interface arguments yet") | |||
This comment has been minimized.
This comment has been minimized.
| @@ -563,6 +563,7 @@ def getJSToNativeConversionInfo(type, descriptorProvider, failureCode=None, | |||
| isMember=False, | |||
| isArgument=False, | |||
| isAutoRooted=False, | |||
| needsRootingInto=False, | |||
This comment has been minimized.
This comment has been minimized.
nox
Mar 6, 2018
Member
Why is this not just reusing isAutoRooted, which AFAIK is initialised from a call to type_needs_auto_root?
This comment has been minimized.
This comment has been minimized.
Xanewok
Mar 6, 2018
Author
Contributor
To briefly summarize from my other comment, the current typed array conversion itself requires prior stack root, whereas argument conversions with isAutoRooted only roots the type after the conversion - to avoid this, it'd need adding unrooted object typed array conversion API and/or supporting rooting those with CustomAutoRooter
|
@bors-servo try |
[Do not merge] WIP: Accept typed array arguments in codegen <!-- Please describe your changes on the following line: --> Preliminary work for #5014. Sorry for a long period of inactivity on the issue. I'd like to return to working on it, however this is more of an exploratory work and approach, rather that something I'm focused on landing. Basically the changes are limited to generating correct conversion codegen using `typedarray` types from rust-mozjs, for function arguments. I believe I need more guidance on how to implement the WebIDL typed array bindings in a more complete/correct way. First of all, these changes do not support [`typedef (ArrayBufferView or ArrayBuffer) BufferSource;`](https://heycam.github.io/webidl/#BufferSource) (or any other union for that matter). I believe that to use them inside regular union bindings code, this will more complete approach wrt rooting and possibly being stored on heap. Currently [`TypedArray`](https://github.com/servo/rust-mozjs/blob/master/src/typedarray.rs) types are only allowed on stack, since they internally hold a stack root guard. Is it possible and if so, how should such a type be stored on heap (in unions?)? Using `RootedTraceableBox`? Secondly, this is the first argument that requires an outer stack root for the argument conversion. Should I add a `TypedArray::from_unrooted` (similar to [`TypedArray::from`](https://github.com/servo/rust-mozjs/blob/master/src/typedarray.rs#L77)) that returns an unrooted 'converted' value, which is only rooted later at the call time, removing the need for newly-added `needsRootingInto` param in codegen? @jdm @KiChjang what do you think about this? ------------ For context, this is the generated code for `void passTypedArray(Int8Array arg);`: ```rust // (...) let mut arg_root = Rooted::new_unrooted(); let arg0: typedarray::Int8Array = if args.get(0).get().is_object() { match typedarray::Int8Array::from(cx, &mut arg_root, args.get(0).get().to_object()) { Ok(val) => val, Err(()) => { throw_type_error(cx, "value is not a typed array."); return false; } } } else { throw_type_error(cx, "Value is not an object."); return false; }; let result: () = this.PassTypedArray(arg0); // (...) ``` --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20205) <!-- Reviewable:end -->
|
|
|
Just to leave few notes regarding future direction, mainly being supported in unions: What SpiderMonkey does is:
From the conversation at #jsapi irc channel, Gecko only supports unions on stack and so that's why it's okay here. However the moment we have an object in nursery that is not traced in a nursery gc (e.g. stored inside some tenured object, but not stack-rooted for nursery gc), then we'd need to use Since codegen generates union types that are not limited to stack lifetime, we'd need to somehow support them being stored with How should it be done?
|
|
I wonder how difficult it would be to move to a model where the generated unions are only valid for stack usage like Gecko? That's probably outside the scope of your work, so I would go with the first option and make typed array generic over the storage type. |
|
Closing in favor of #20267. |
…newok Support JS typed arrays as arguments and in WebIDL unions <!-- Please describe your changes on the following line: --> Supersedes #20205. This brings support to receiving typed arrays as function arguments (those are stack-rooted with CustomAutoRooter) and also being a member of a union (which is basically heap-rooted? similarly to other webidl unions). This is based on my other PR #20265 (which means it has to pull an external rust-mozjs branch and contains some also slightly unrelated changes here) since it shares `RootedTraceableBox::from_box` here and some other additional changes at rust-mozjs, but it can be easily separated if needed. I tried adding support to nullable typed arrays but couldn't work around an issue of codegen always sticking a "OrNull" at the end of my type (presumably because of [this](https://github.com/servo/servo/blob/master/components/script/dom/bindings/codegen/parser/WebIDL.py#L2241)?). How would I go about avoiding the suffix with nullable arguments? If we were to add also support for nullable typed arrays then I think we wouldn't be blocked anymore on this in WebGL 1.0. r? @jdm --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20267) <!-- Reviewable:end -->
Xanewok commentedMar 5, 2018
•
edited
Preliminary work for #5014.
Sorry for a long period of inactivity on the issue. I'd like to return to working on it, however this is more of an exploratory work and approach, rather that something I'm focused on landing.
Basically the changes are limited to generating correct conversion codegen using
typedarraytypes from rust-mozjs, for function arguments.I believe I need more guidance on how to implement the WebIDL typed array bindings in a more complete/correct way.
First of all, these changes do not support
typedef (ArrayBufferView or ArrayBuffer) BufferSource;(or any other union for that matter).I believe that to use them inside regular union bindings code, this will more complete approach wrt rooting and possibly being stored on heap. Currently
TypedArraytypes are only allowed on stack, since they internally hold a stack root guard. Is it possible and if so, how should such a type be stored on heap (in unions?)? UsingRootedTraceableBox?Secondly, this is the first argument that requires an outer stack root for the argument conversion. Should I add a
TypedArray::from_unrooted(similar toTypedArray::from) that returns an unrooted 'converted' value, which is only rooted later at the call time, removing the need for newly-addedneedsRootingIntoparam in codegen?@jdm @KiChjang what do you think about this?
For context, this is the generated code for
void passTypedArray(Int8Array arg);:./mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThis change is