Use deep copy for setting internal slots? #395
Comments
|
ECMAScript doesn't really shed light on this. Internal slots are supposed to be created and then not modified unless specified differently. Now we seem to have 2 options:
@anssiko @dontcallmedom any input here? |
|
If we need to do this in many places maybe it makes sense to define an abstract operation for doing a deep copy? Did you check whether any of the infra specs incl. HTML have reusable helpers for this? For some inspiration, for structured data there's a bunch of abstract operations in HTML https://html.spec.whatwg.org/multipage/structured-data.html referenced by other specs such as IndexedDB, Web Workers. |
|
Yes, we can define separate "set internal slot" steps for that and when it becomes specified in INFRA or ECMA, we can switch. What we need is to prevent changes from client after setting, i.e. to pass ownership of the init dict to the internal slot's object. An emulation of that is [deep] copy / pass by value, usually referenced as "copy of" in WebIDL, as suggested above. |
|
Yes, I think a PR for that option would help. |
|
Since it affects all internal slots and the whole spec, I will do it after the current batch of PRs is merged. |
|
Setting internal slots has been explained here. |
|
I've been going through the spec and I don't see anywhere where an internal slot holds a non-primitive data type, and the properties are mutated after it is set. Most specifically, once a MLOperand's [[descriptor]] is set I never see it changing. The closest I've spotted is in resample output sizes where the input descriptor is cloned then mutated with:
This could be made more explicit with "Let desc be a new MLOperandDescriptor..." Otherwise... I haven't spotted any usage that would require more explicit deep copies. Am I missing any? FWIW, I was "grepping the spec" using this on the console: |
* Per webmachinelearning#395 make it clearer that the algorithm is working on a new descriptor (a copy of the input's) * When invoking the algorithm, pass the required input argument. * Rename algorithm from "resample output sizes" to "calculate resample output sizes" which more clearly descripts what is happening and aligns with similar algorithms in the spec.
|
The calculate matmul output sizes may also be misleading, as it does:
The intent here is a copy, not mutating the input's descriptor. |
For gemm() - make lists that are mutated be explicit clones using Infra terminology, and simplify the wording reversing the lists. For matmul() - make lists that are mutated be explicit clones using Infra terminology, use append/prepend definitions from Infra, convert a variable change from "let" to "set" and drop use of "array". For webmachinelearning#395
For gemm() - make lists that are mutated be explicit clones using Infra terminology, and simplify the wording reversing the lists. For matmul() - make lists that are mutated be explicit clones using Infra terminology, use append/prepend definitions from Infra, convert a variable change from "let" to "set" and drop use of "array". For webmachinelearning#395
For gemm() - make lists that are mutated be explicit clones using Infra terminology, and simplify the wording reversing the lists. For matmul() - make lists that are mutated be explicit clones using Infra terminology, use append/prepend definitions from Infra, convert a variable change from "let" to "set" and drop use of "array". For webmachinelearning#395
For gemm() - make lists that are mutated be explicit clones using Infra terminology, and simplify the wording reversing the lists. For matmul() - make lists that are mutated be explicit clones using Infra terminology, use append/prepend definitions from Infra, convert a variable change from "let" to "set" and drop use of "array". For webmachinelearning#395
* Per webmachinelearning#395 make it clearer that the algorithm is working on a new descriptor (a copy of the input's) * When invoking the algorithm, pass the required input argument. * Rename algorithm from "resample output sizes" to "calculate resample output sizes" which more clearly descripts what is happening and aligns with similar algorithms in the spec, and add types.
* Per #395 make it clearer that the algorithm is working on a new descriptor (a copy of the input's) * When invoking the algorithm, pass the required input argument. * Rename algorithm from "resample output sizes" to "calculate resample output sizes" which more clearly descripts what is happening and aligns with similar algorithms in the spec, and add types.
For gemm() - make lists that are mutated be explicit clones using Infra terminology, and simplify the wording reversing the lists. For matmul() - make lists that are mutated be explicit clones using Infra terminology, use append/prepend definitions from Infra, convert a variable change from "let" to "set" and drop use of "array". For webmachinelearning#395
…achinelearning#520) * Per webmachinelearning#395 make it clearer that the algorithm is working on a new descriptor (a copy of the input's) * When invoking the algorithm, pass the required input argument. * Rename algorithm from "resample output sizes" to "calculate resample output sizes" which more clearly descripts what is happening and aligns with similar algorithms in the spec, and add types.
For gemm() - make lists that are mutated be explicit clones using Infra terminology, and simplify the wording reversing the lists. For matmul() - make lists that are mutated be explicit clones using Infra terminology, use append/prepend definitions from Infra, convert a variable change from "let" to "set" and drop use of "array". For webmachinelearning#395
For gemm() - make lists that are mutated be explicit clones using Infra terminology, and simplify the wording reversing the lists. For matmul() - make lists that are mutated be explicit clones using Infra terminology, use append/prepend definitions from Infra, convert a variable change from "let" to "set" and drop use of "array". For webmachinelearning#395
For gemm() - make lists that are mutated be explicit clones using Infra terminology, and simplify the wording reversing the lists. For matmul() - make lists that are mutated be explicit clones using Infra terminology, use append/prepend definitions from Infra, convert a variable change from "let" to "set" and drop use of "array". For webmachinelearning#395
* Wording change: Tighten up output shape calculation algorithms For gemm() - make lists that are mutated be explicit clones using Infra terminology, and simplify the wording reversing the lists. For matmul() - make lists that are mutated be explicit clones using Infra terminology, use append/prepend definitions from Infra, convert a variable change from "let" to "set" and drop use of "array". For #395 * Rewrite to match Chromium impl * Update index.bs Co-authored-by: Dwayne Robinson <[email protected]> --------- Co-authored-by: Dwayne Robinson <[email protected]>
|
@zolkis - do you think we can close this out? Or can you spot any other changes we should make? |
|
@inexorabletash yes, we can close it, thanks for the lots of help here. |
Originally posted by @huningxin in #348 (comment)
The text was updated successfully, but these errors were encountered: