TypeScript support #70

lu4 · 2019-06-10T00:02:00Z

This PR is a major rewrite for the library, it introduces TypeScript support for node-opencl. Suggested changes are not backward compatible, there are changes in package.json, and changes in both C++ backend interface and NodeJS frontend interface, also MR contains couple of bugfixes, also there are changes in ... ... ... everywhere. I think this PR requires broader attention and some time to sync with existing code base. Or at least something to start with...

Hope to hear out your comments!

Thank you in advance!

…penCL 1.2

…eseven/node-opencl into feature/upgrade_to_NAN_2.8.0

…ntal OpenCL 2.1 CPU Only Platform

…/49216021)

* return compiled program from clGetProgramInfo(cl.PROGRAM_BINARIES) * detect if a buffer is being unwrapped inside NoCLWrapper::Unwrap, and reinterpret_cast into the type if so * only pass programs with size > 0 to clCreateProgramFromBinary for nvidia

…h older node versions

Intermediate commit

lmeyerov · 2019-11-04T21:32:05Z

@lu4 - picking this back up

Does this preserve API compatibility with non-ts users? if not, can it be done in a way that does so, such as alt calls?

lu4 · 2019-11-05T21:26:28Z

Hi @lmeyerov, I'm having trouble with understanding what kind of compatibility are you asking about? Do you mean whether JavaScript developers would be able to use the provided api without the need of migrating to TypeScript? (If so than yes, they should have such ability, but this support might require couple of checks from my side and probably couple of updates. If this is the compatibility you are asking about then let me know)

@lu4 - picking this back up

Does this preserve API compatibility with non-ts users? if not, can it be done in a way that does so, such as alt calls?

lmeyerov · 2019-11-05T21:35:10Z

Yes, exactly:

-- can new node-opencl users use it without using typescript themselves?
-- were there no public api changes, so existing node-opencl users can keep using it with no changes?

lmeyerov · 2019-11-05T21:38:34Z

If both are a yes, will help test + land soon after #69 . . If a no... bet we can figure it out :)

lu4 · 2019-11-05T22:04:07Z

-- can new node-opencl users use it without using typescript themselves?
--> In theory yes, but appropriate checks / preparations should happen before the answer becomes a definitive yes

-- were there no public api changes, so existing node-opencl users can keep using it with no changes?
--> It is hard for me to remember at this point all of the changes, but off-top of my head I've fixed couple of bugs I've found myself which in theory may break backward compatibility. As for public interface I've tried to stick as close as possible to both the original node-opencl design and official Khronos spec, and so the places where I've found severe discrepancy between the two, I've tried to improve (if such improvement was possible), these changes may also break the backward compatibility. The changes I remember are:

Int64 support (I made it little bit different, right now I don't remember how exactly I've implemented, but since then BigInt has became official part of ES standard which is better candidate than a pair of low / high numbers)
I've made deprecated OpenCL functions available (it shouldn't affect existing users, there's no need to be so restrictive about deprecated APIs)

Maybe there's something else, I don't remember. But what I can say for sure is that most of these changes would most probably affect major version of node-opencl.

In any case I'm open to help with transition (full or parts) but it'll definitely take some time to happen...

lmeyerov · 2019-11-08T07:35:18Z

Gotcha, thanks! We're seeing some fires on the new AWS G4s, which is ~4x cheaper than any of the others, so that shot up in priority. After we figure that out (1w?), will try this & report back!

lmeyerov · 2019-11-23T06:22:01Z

Hi @lu4 , just got commit access & merged the non-TS branch, and happy to test & land this one when you think it's ready.

lmeyerov · 2019-11-23T06:24:18Z

RE:Breaking changes, esp. for increased conformance & coverage , sounds worthwhile, we can just do a major version bump & call out in a changelog.

lu4 · 2019-11-23T06:48:30Z

@lmeyerov sounds cool, I'm experimenting with embind, ffi, genapi, nbind for other project these tools allow for automatic/semi-automatic bindings generation and look promising, but as I suspect we won't be throwing away existing code. So will signal you when ready, not promising anything, but hope to complete in couple of days

lmeyerov

Went through individual files, will post full msg on main thread

lmeyerov · 2019-11-23T06:32:50Z

.vscode/launch.json

@@ -0,0 +1,55 @@
+{
+    // Use IntelliSense to learn about possible attributes.


Invalid json?

This is default Visual Studio Code launch.json config file, I think it is reasonable to remove it

lmeyerov · 2019-11-23T06:33:43Z

README.md

+
+I've taken the initiative to renovate the existing code for my own purposes and now I'm sharing the result in hope that it will be useful for anyone else who is lacking typescript support with `node-opencl`. 
+
+This project contains full typings coverage of OpenCL 1.0-2.0 (and OpenCL 2.1, 2.2 ready to be implemented) specification (based on Khronos specification) with node-opencl specifics taken into account. The package also contains couple of minor bugfixes suggested by community PRs in original repo and couple of mine aesthetic interface changes / additions and changes related to management of deprecated OpenCL apis (basically I enabled the use of deprecated APIs if they were still supported by OpenCL implementation). Which I think may block mikeseven from merging current changes into `node-opencl`. Nevertheless in order to not partition the community with redundant library versions I'm open to kill this repo in event of merge with `node-opencl`.


Can we move the TS-specific docs to a subsection?

I think we can stick to original readme, or have new one created, no need for message above

lmeyerov · 2019-11-23T06:37:02Z

package.json

@@ -1,11 +1,13 @@
 {
-  "name": "node-opencl",
-  "version": "0.4.5",
+  "name": "node-opencl-ts",


Let's make this as part of core node-opencl, unless a distinct package is a convention in ts?

Will rename this to original package name

lmeyerov · 2019-11-23T06:37:42Z

package.json

  ],
  "license": "BSD-3-Clause",
  "repository": {
    "type": "git",
-    "url": "https://github.com/mikeseven/node-opencl.git"
+    "url": "https://github.com/lu4/node-opencl-ts.git"


s/lu4/mikeseven...

lmeyerov · 2019-11-23T06:37:57Z

package.json

  },
  "bugs": {
-    "url": "https://github.com/mikeseven/node-opencl/issues"
+    "url": "https://github.com/lu4/node-opencl-ts/issues"


lmeyerov · 2019-11-23T07:11:50Z

test/test.device.js

-      testString(device, "DEVICE_BUILT_IN_KERNELS");
-      testString(device, "DEVICE_SPIR_VERSIONS");
+      // testString(device, "DEVICE_BUILT_IN_KERNELS");
+      // testString(device, "DEVICE_SPIR_VERSIONS");


should not skip tests... better to guard tests by hw build?

I'll rethink this

lmeyerov · 2019-11-23T07:12:48Z

test/test.event.js

@@ -27,7 +27,7 @@ describe("Event", function() {

  describe("#getEventInfo",function(){
    function testNumber(info,name,expected) {
-      it("should return the good value for " + name, function () {
+      skip().vendor("nVidia").it("should return the good value for " + name, function () {


more skipped tests

I see more need of incremental migration to TypeScript support.

lmeyerov · 2019-11-23T07:13:19Z

test/test.kernel.js

@@ -138,11 +140,11 @@ describe("Kernel", function () {
        U.withProgram(ctx, squareKern, function (prg) {
          var k = cl.createKernel(prg, "square");

-          if (cl.VERSION_1_2) {
-            U.bind(cl.setKernelArg, k, 0, [5, 10, 15])
+          if (cl.v12) {


more v12 refactors, not sure why

I can return back the naming but except for naming itself there are changes related to contents of namespaces, node-opencl does not provide deprecated APIs on javascript side, it is literally excluded during C++ compile time, I think this is wrong thing to do, if API is there it should be available. As for reasons to change naming is that I find 'v12' more convenient to write, no need to shout VERSION (press shift) (press minus) 1 (press shift) (press minus) 2

lmeyerov · 2019-11-23T07:13:53Z

test/test.kernel.js

          }
-          assert(cl.setKernelArg(k, 2, 5, "uint") == cl.SUCCESS);
+          assert(cl.setKernelArg(k, 2, "uint", 5) == cl.SUCCESS);


looks like kernel arg order changed, call out?

lmeyerov · 2019-11-23T07:15:08Z

test/test.memobj.js

@@ -402,47 +404,47 @@ describe("MemObj", function() {
      });
    });

-    it("should get supported image formats", function () {
+    skip().vendor('nVidia').it("should get supported image formats", function () {


don't have an answer now

lmeyerov · 2019-11-23T07:35:07Z

Wow, went through all core files and most of tests, this is great!

AFAICT, this:

-- Adds TS 🚀🚀🚀
-- More nan upgrades 💪💪💪
-- Regresses on callback support?
-- Disables a bunch of tests? 😔
-- Some conformance fixes: some arg orders, ... 💪💪
-- A few unnecessary refactors: CL_1_2 => v12, getInfo signature change around byte rep, ...
-- Some file moves, some of which aren't version-tracked
-- README/package.json changes from while this was a fork

My thinking to get landed:
-- Need to document the breaking API-level changes. IMO, fine to do a least-effort solution like a migration section at the bottom of the main readme.
-- May be smart to back out or split into a diff PR non-TS breaking changes ones such as the .info -- low value, mostly incurred work on others (e.g., we use that for some user-level scheduling, so annoying to port due to low value)
-- What's going on w/ the event args? Looks like they got cut some reason?
-- OK to lose version history on test/examples
-- Tests should be reenabled
-- I'm not a great reviewer of some of the C++ stuff, which compounds pain of a massive PR, but this is hard piecemeal so I get it :)

So maybe the flow is:
-- Think about ^^^^
-- Address what is reasonable
-- When ready, I can help basic AWS nvidia linux testing + osx intel
-- Once we're happy with that, I can smoke test on our full production workloads
-- ... and help with npm publishing (will figure out that flow w/ current branch)
-- If the regressions get too hard to land immediately, we can start by doing a TS branch

Sounds reasonable / ideas?

Also, we can probably donate an Azure Pipelines CI testing across AWS intel/nvidia, if helpful

lmeyerov · 2019-11-23T07:41:31Z

@lmeyerov sounds cool, I'm experimenting with embind, ffi, genapi, nbind

Cool! Yeah, I'm cool with most things that eliminate code and improve maintainability. Some of those look like quite risky dependencies that'll bite us in 6mo so rather not take on the likely coming tech debt, but beyond that, am supportive.

lu4 · 2019-11-26T15:23:40Z

Ok, I'm back, will review and respond to your comments, will create a checklist and gradually will fix them...

lu4 · 2019-11-26T17:01:39Z

Ok, I've reviewed all your comments, I see that it might be too hard to try to move the new thing on top of old thing, there are too many layers of changes. Let's do it this way, I see that it's possible to introduce TypeScript bindings into existing project without touching the C++ side. I'll do this in a separate PR, and as of that we can start moving towards C++ changes (if necessary)

I'm saying "if necessary" because I'd rewrite the whole thing from scratch, I don't like most of the things I see, but unfortunately I don't have time for such adventure. So let's go forward with this step by step... What are your thoughts on this?

lmeyerov · 2019-11-26T17:59:41Z

Yes, splitting it up would make this a lot more approachable!

Maybe something like:

Prioritize an initial TS PR as a minor semvar bump:
-- Make the TS match the existing signatures. If any breaking changes, make an explicit and well-motivated migration list
-- I can help turn the list into an upgrade guide + help w/ device & integration testing
-- => Most people can start using with high confidence and upgrade with little pain

2a. C++ changes (nan, ...) as a diff PR, ideally also as a minor semvar that conforms to (1)'s TS
=> same benefits as (1), and easier to do the security etc. review

2b. New abstractions (eg., async/await) as a major semvar, and maybe have live as an @next for awhile => "node-opencl 2.0"

EDIT: Should I respond to the comments, or do anything else to help for next steps?

mikeseven and others added 30 commits November 23, 2017 22:03

upgraded to latest NAN 2.8.0, checked tests on MacBook Pro's crappy O…

54712c7

…penCL 1.2

updated with CL 2.1 headers (methods not implemented yet)

89f006d

updated with CL 2.2 header (no method implementation yet)

3f9d6c0

cleaned up deprecated warnings with v8 and nan

7d2de1f

updated examples to support OCL 1.2 and 2. Tested on Mac (opencl 1.2).

9bdbdda

blacklisted some tests not working, buggy driver, on Mac OS X.

ed2ccbd

move some things from dependencies to devDependencies

69507c0

upgraded to latest NAN 2.8.0, checked tests on MacBook Pro's crappy O…

0393dde

…penCL 1.2

updated with CL 2.1 headers (methods not implemented yet)

efda566

updated with CL 2.2 header (no method implementation yet)

6413147

cleaned up deprecated warnings with v8 and nan

f1fbff9

updated examples to support OCL 1.2 and 2. Tested on Mac (opencl 1.2).

aeea819

blacklisted some tests not working, buggy driver, on Mac OS X.

964da8b

Merge branch 'feature/upgrade_to_NAN_2.8.0' of https://github.com/mik…

209cb50

…eseven/node-opencl into feature/upgrade_to_NAN_2.8.0

schedule AsyncQueueWorker to be called on the main thread

50c2981

support cl_nv_device_attribute_query

c36300c

update withContext test util to use opencl2 createContextWithType

4fcad23

use updated node-memwatch, cleanup test_leak1 test

6adb7ec

fix test.svm tests for intel-opencl 2.1

5f391a8

add vscode debugging launch configuration

515d27b

fix createImage host_ptr argument index

3e54df5

fix or ignore failing tests to make everything pass in Intel Experime…

f2c7c41

…ntal OpenCL 2.1 CPU Only Platform

skip some tests that segfault on nvidia pascal

8a825a2

fix node v12.0.0 compilation errors

6a5f1d4

fix -Wdeprecated-declarations warnings

64afb34

ignore gcc std::vector alignment warning (https://stackoverflow.com/a…

938ba1f

…/49216021)

add scripts to debug c++ in vscode

9062771

cleanup more cl objects in tests

d73b663

use nan@^2.13.2

8070411

trxcllnt and others added 7 commits April 29, 2019 11:11

use FromJust() instead of ToChecked() for backwards-compatibility wit…

c64da8d

…h older node versions

Add missing Nan support, for sampler.cpp, less than OpencCL version 2

87c4e63

Minor code quality improvements

f029a1c

Based on mikeseven@3603302

39c4486

TypeScript support, initial commit

33f26f9

Intermediate commit

140c2cc

Merge pull request #1 from lu4/TypeScriptSupport

9dc971e

Intermediate commit

lmeyerov requested changes Nov 23, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TypeScript support #70

TypeScript support #70

lu4 commented Jun 10, 2019

lmeyerov commented Nov 4, 2019

lu4 commented Nov 5, 2019

lmeyerov commented Nov 5, 2019

lmeyerov commented Nov 5, 2019

lu4 commented Nov 5, 2019

lmeyerov commented Nov 8, 2019

lmeyerov commented Nov 23, 2019

lmeyerov commented Nov 23, 2019

lu4 commented Nov 23, 2019

lmeyerov left a comment

lmeyerov Nov 23, 2019

lu4 Nov 26, 2019 •

edited

Loading

lmeyerov Nov 23, 2019

lu4 Nov 26, 2019

lmeyerov Nov 23, 2019

lu4 Nov 26, 2019

lmeyerov Nov 23, 2019

lmeyerov Nov 23, 2019

lmeyerov Nov 23, 2019

lu4 Nov 26, 2019

lmeyerov Nov 23, 2019

lu4 Nov 26, 2019

lmeyerov Nov 23, 2019

lu4 Nov 26, 2019 •

edited

Loading

lmeyerov Nov 23, 2019

lu4 Nov 26, 2019

lmeyerov Nov 23, 2019

lu4 Nov 26, 2019

lmeyerov commented Nov 23, 2019 •

edited

Loading

lmeyerov commented Nov 23, 2019

lu4 commented Nov 26, 2019

lu4 commented Nov 26, 2019

lmeyerov commented Nov 26, 2019 •

edited

Loading

		@@ -0,0 +1,55 @@
		{
		// Use IntelliSense to learn about possible attributes.


		I've taken the initiative to renovate the existing code for my own purposes and now I'm sharing the result in hope that it will be useful for anyone else who is lacking typescript support with `node-opencl`.

		This project contains full typings coverage of OpenCL 1.0-2.0 (and OpenCL 2.1, 2.2 ready to be implemented) specification (based on Khronos specification) with node-opencl specifics taken into account. The package also contains couple of minor bugfixes suggested by community PRs in original repo and couple of mine aesthetic interface changes / additions and changes related to management of deprecated OpenCL apis (basically I enabled the use of deprecated APIs if they were still supported by OpenCL implementation). Which I think may block mikeseven from merging current changes into `node-opencl`. Nevertheless in order to not partition the community with redundant library versions I'm open to kill this repo in event of merge with `node-opencl`.

TypeScript support #70

Are you sure you want to change the base?

TypeScript support #70

Conversation

lu4 commented Jun 10, 2019

lmeyerov commented Nov 4, 2019

lu4 commented Nov 5, 2019

lmeyerov commented Nov 5, 2019

lmeyerov commented Nov 5, 2019

lu4 commented Nov 5, 2019

lmeyerov commented Nov 8, 2019

lmeyerov commented Nov 23, 2019

lmeyerov commented Nov 23, 2019

lu4 commented Nov 23, 2019

lmeyerov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lu4 Nov 26, 2019 • edited Loading

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

lu4 Nov 26, 2019 • edited Loading

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

lmeyerov commented Nov 23, 2019 • edited Loading

lmeyerov commented Nov 23, 2019

lu4 commented Nov 26, 2019

lu4 commented Nov 26, 2019

lmeyerov commented Nov 26, 2019 • edited Loading

lu4 Nov 26, 2019 •

edited

Loading

lu4 Nov 26, 2019 •

edited

Loading

lmeyerov commented Nov 23, 2019 •

edited

Loading

lmeyerov commented Nov 26, 2019 •

edited

Loading