Add a helper method to copy an array of numbers to a JS TypedArray #31

j-f1 · 2020-08-04T18:16:39Z

Extracted from #26.

Sources/_CJavaScriptKit/include/_CJavaScriptKit.h

MaxDesiatov

Does DOMKit need this for anything? If so, wouldn't there be a use case for conversion in the other direction, from JS to Swift? Do I understand correctly that it's not supported in this PR? If it is planned, it would be totally fine (preferred even) to land that as a separate PR, just wanted to clarify it.

j-f1 · 2020-08-04T19:02:28Z

I did a quick search, and it seems like a TypedArray is used as input to the Blob constructor. There’s also ways to construct TypedArrays out of the ArrayBuffer you get back from a Blob. The TypedArray stuff is separate from the main part of DOMKit, and there is no support for creating one from an ArrayBuffer yet, so we could push that feature to a future PR.

j-f1 · 2020-08-04T19:11:33Z

Also happy to write something that goes the other way now, though.

kateinoigakukun · 2020-08-09T04:00:10Z

Runtime/src/index.ts

+                elementsPtr: pointer, length: number,
+                result_obj: pointer
+            ) => {
+                const ArrayType: TypedArray = this.heap.referenceHeap(0)[JavaScriptTypedArrayKind[kind] + 'Array']


Please don't abuse address 0 just to access global context.
It would be better to store global or window as a member of SwiftRuntime and access it directly 👍

I created a globalVariable variable as a replacement. Is that a good name or should I go with something else?

Runtime/src/index.ts

IntegrationTests/TestSuites/Sources/PrimaryTests/main.swift

Sources/JavaScriptKit/JSObject.swift

j-f1 · 2020-08-13T15:28:44Z

I’ve created a JSTypedArray protocol that all the concrete types inherit from. Should I do it this way (matching the JS API) or just make JSTypedArray a generic class?

…h test errors on the JS side happen in

This matches the Objective-C names and better corresponds with their tasks (freeHeap just decreases ref count so it doesn’t always free). I’m not as sure about allocHeap → retain because it adds to the heap in addition to increasing the refcount.

Previously it would just return undefined which would throw an error when attempting to use the object later on. Now you’re able to demangle the stack trace to find exactly where you’re misusing a ref.

j-f1 · 2020-08-15T16:58:26Z

Another consideration: While working on #26, I changed the JSTypedArray class to wrap a regular JSObjectRef (like JSArrayRef does) instead of extending it (like JSFunction does). That makes use of a JSBridgedClass type which I could implement in a separate PR. Which approach is better?

kateinoigakukun

First of all, I'm not confident with the current interface design.

When bridging JavaScript object into Swift, the class inheritance relationship information is not given into Swift world.
So they are flattened into JSObjectRef and users need to use JavaScript cast outside of Swift system. (In this case, JavaScript cast means a cast that checks the type of value at runtime using instanceof when casting like JSArrayRef.)

But for JSFunctionRef, we need to consider it as a special class because JavaScript cast depends on JSFunction itself. For example, if JavaScriptKit treats JSFunction as well as JSArrayRef, when casting a JSObjectRef into JSFunctionRef, JavaScript cast system needs to check that the object is a function using obj instanceof Function. But how can we get Function class? It can't.

For this reason, JSFunctionRef and JSObjectRef should be distinguished when taking it from JavaScript.

I'd like to see the JSBridgedClass impl. Could you send another PR to review it?

kateinoigakukun · 2020-08-16T03:21:31Z

Sources/JavaScriptKit/JSTypedArray.swift

+    }
+
+    public init(length: Int) {
+        let jsObject = Element.typedArrayClass.new(length)


How about having the jsObject in JSTypedArray instead of manual reference count management?

kateinoigakukun · 2020-08-16T03:24:19Z

Runtime/src/index.ts

@@ -8,6 +8,17 @@ type pointer = number;
 interface GlobalVariable { }
 declare const window: GlobalVariable;
 declare const global: GlobalVariable;
+let globalVariable: any;


I think this variable can be capsulized in SwiftRuntimeHeap.

Runtime/src/index.ts

kateinoigakukun · 2020-08-16T04:48:07Z

I’ve created a JSTypedArray protocol that all the concrete types inherit from. Should I do it this way (matching the JS API) or just make JSTypedArray a generic class?

I prefer current generic class version. 👍

j-f1 · 2020-08-16T12:17:34Z

I'd like to see the JSBridgedClass impl. Could you send another PR to review it?

It’s not 100% clean yet (there’s a few other things in the PR that are unrelated) but I added it in #26. (see diff between it and this PR)

There’s the JSBridgedType protocol which adds an init?(from: JSValue) initializer, which returns nil if the given value isn’t supported by the implementation. It extends JSValueCodable (aka JSValueConvertible & JSValueConstructible) so you can convert it back and forth from a JSValue using those APIs [maybe I should change it so that JSValueConvertible uses a jsValue read-only property and you do SomeJSValueConstructible(from: JSValue) instead of having a static func construct method?]

Then there’s the JSBridgedClass protocol which has a static classRef property pointing to the JS constructor for the class being represented and an instance objectRef property containing the JSObjectRef being wrapped. The init?(from: JSValue) implementation in JSBridgedClass ensures that the passed value is an object and is an instance of the classRef before calling init(withCompatibleObject: JSObjectRef) to delegate to the specific implementation.

For example, if JavaScriptKit treats JSFunction as well as JSArrayRef, when casting a JSObjectRef into JSFunctionRef, JavaScript cast system needs to check that the object is a function using obj instanceof Function. But how can we get Function class? It can't.

I might be misunderstanding this but there is a Function property on the JS global object that all functions (including generator, async, and arrow functions) are instanceof. I do think it makes sense to treat functions differently since they have a different typeof though.

MaxDesiatov · 2020-09-08T09:37:38Z

I'm thinking of generating some WebGLKit code, thankfully an IDL file for it is available. Using typed arrays is pretty much a requirement for WebGL, so I'd be very happy for this PR to move forward. Is there any way I can help here?

j-f1 · 2020-09-08T12:57:09Z

The main sticking point is on the bridging API. I think the one in #26 can be extracted out to another PR, if @kateinoigakukun likes it.

kateinoigakukun · 2020-09-09T02:40:46Z

This is my change proposal j-f1#1

If you accept my proposal, I can immediately merge your PR #31
In the future, I also want to implement CoW system on typed-array to avoid heavy copy but it's out of scope of this PR.

Typed array change proposal

Add a helper method to copy an array of numbers to a JS TypedArray

fe68a31

j-f1 force-pushed the typed-array branch from 2962215 to fe68a31 Compare August 4, 2020 18:17

_copy_typed_array_content → _create_typed_array

0cb3f65

MaxDesiatov reviewed Aug 4, 2020

View reviewed changes

Sources/_CJavaScriptKit/include/_CJavaScriptKit.h Outdated Show resolved Hide resolved

MaxDesiatov reviewed Aug 4, 2020

View reviewed changes

kateinoigakukun requested changes Aug 9, 2020

View reviewed changes

j-f1 added 4 commits August 12, 2020 17:14

Merge remote-tracking branch 'upstream/master' into typed-array

3eca18c

Add globalVariable

bff8568

Remove broken test target

136315f

Create JSTypedArray

5b875b2

j-f1 added 2 commits August 13, 2020 11:40

Reduce to just a single class

af57583

Clean up types

e71fc56

MaxDesiatov requested a review from kateinoigakukun August 13, 2020 16:25

j-f1 added 12 commits August 13, 2020 15:33

Fix tests

17d83f5

Formatting

64342d2

Test all the array types

ab974af

Fix test error

0928da8

Add a test("name") { ... } helper that makes it easy to find out whic…

a1f5b03

…h test errors on the JS side happen in

Propagate names through to the Swift side

dde8cf2

Add an explicit retain() function and fix a ref counting bug

74610c2

Add error when reading invalid reference

14ab088

Previously it would just return undefined which would throw an error when attempting to use the object later on. Now you’re able to demangle the stack trace to find exactly where you’re misusing a ref.

Actually fix the tests

b6602c8

Explain why _retain is necessary

561b8a6

Update _CJavaScriptKit.h

b0ff949

j-f1 mentioned this pull request Aug 15, 2020

Add JSBridgedType and JSBridgedClass #26

Merged

kateinoigakukun reviewed Aug 16, 2020

View reviewed changes

j-f1 force-pushed the typed-array branch from 8ceeaeb to b0ff949 Compare August 21, 2020 01:23

Merge remote-tracking branch 'upstream/master' into typed-array

7836ac2

kateinoigakukun added 3 commits September 9, 2020 11:05

Merge branch 'master' into typed-array-change-proposal

e0ef55f

Remove manual reference counting

d64def7

Fix test cases

fab45e1

kateinoigakukun mentioned this pull request Sep 9, 2020

Typed array change proposal j-f1/forked-JavaScriptKit#1

Merged

kateinoigakukun and others added 2 commits September 10, 2020 15:58

Expose failable initializer

f83a84c

Merge pull request #1 from kateinoigakukun/typed-array-change-proposal

2370a1f

Typed array change proposal

kateinoigakukun approved these changes Sep 10, 2020

View reviewed changes

kateinoigakukun merged commit 171c391 into swiftwasm:master Sep 10, 2020

j-f1 deleted the typed-array branch September 12, 2020 23:45

j-f1 mentioned this pull request Sep 16, 2020

Remove JavaScriptTypedArrayKind #58

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a helper method to copy an array of numbers to a JS TypedArray #31

Add a helper method to copy an array of numbers to a JS TypedArray #31

j-f1 commented Aug 4, 2020

MaxDesiatov left a comment

j-f1 commented Aug 4, 2020

j-f1 commented Aug 4, 2020

kateinoigakukun Aug 9, 2020

j-f1 Aug 12, 2020

j-f1 commented Aug 13, 2020

j-f1 commented Aug 15, 2020 •

edited

Loading

kateinoigakukun left a comment •

edited

Loading

kateinoigakukun Aug 16, 2020

kateinoigakukun Aug 16, 2020

kateinoigakukun commented Aug 16, 2020

j-f1 commented Aug 16, 2020

MaxDesiatov commented Sep 8, 2020

j-f1 commented Sep 8, 2020

kateinoigakukun commented Sep 9, 2020 •

edited

Loading

Add a helper method to copy an array of numbers to a JS TypedArray #31

Add a helper method to copy an array of numbers to a JS TypedArray #31

Conversation

j-f1 commented Aug 4, 2020

MaxDesiatov left a comment

Choose a reason for hiding this comment

j-f1 commented Aug 4, 2020

j-f1 commented Aug 4, 2020

kateinoigakukun Aug 9, 2020

Choose a reason for hiding this comment

j-f1 Aug 12, 2020

Choose a reason for hiding this comment

j-f1 commented Aug 13, 2020

j-f1 commented Aug 15, 2020 • edited Loading

kateinoigakukun left a comment • edited Loading

Choose a reason for hiding this comment

kateinoigakukun Aug 16, 2020

Choose a reason for hiding this comment

kateinoigakukun Aug 16, 2020

Choose a reason for hiding this comment

kateinoigakukun commented Aug 16, 2020

j-f1 commented Aug 16, 2020

MaxDesiatov commented Sep 8, 2020

j-f1 commented Sep 8, 2020

kateinoigakukun commented Sep 9, 2020 • edited Loading

j-f1 commented Aug 15, 2020 •

edited

Loading

kateinoigakukun left a comment •

edited

Loading

kateinoigakukun commented Sep 9, 2020 •

edited

Loading