Skip to content

Conversation

@kjvalencik
Copy link

@kjvalencik kjvalencik commented Sep 15, 2021

Hello!

I'm a maintainer on the Neon project and we are considering a couple of major [breaking] changes as we ramp up to the v1.0 release. Since skia-canvas is a significant user of Neon, I wanted to get your thoughts.

Two proposed changes that impact this project are the rewritten Borrow API and JsPromise/Tasks.

For the most part, these are cosmetic changes and should not impact how the code executes. However, there is one significant difference in this PR. Previously, async tasks were being spawned using rayon::spawn which uses the rayon threadpool. However, this uses cx.task(...) which uses the Node worker pool.

Since these are CPU intensive tasks, I don't expect it to have a significant difference; however, I'm not sure if rayon optimizes execution from its own threads better than other pools. If the previous functionality is preferred, the implementation can be swapped to cx.channel() and Channel::settle_with.

I would appreciate your thoughts. Thanks so much for the support!

Note: This PR is only meant as a discussion topic and should not be used. It uses an alpha version of Neon.


let promise = canvas.png
expect(promise).toBeInstanceOf(Promise)
expect(util.types.isPromise(promise))
Copy link
Author

@kjvalencik kjvalencik Sep 15, 2021

Choose a reason for hiding this comment

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

I'm not sure why the Promise returned by Node-API is a different class than global.Promise. It might have something to do with jest? When I test a simple case locally promiseFromNeon instanceof Promise is true.

@samizdatco
Copy link
Owner

samizdatco commented Sep 22, 2021

I'm a wholehearted fan of the TypedArray support (and am grateful for any opportunity to remove a cx.borrow(…, ||) closure).

On the promise/task side of things, the syntax is really nice—quite the improvement! My only concern is a little wariness about having two separate thread pools potentially fighting with one another. Given how likely it is for client code to be using rayon, I can see an argument for keeping as much of neon's background processing in that same hopper if it means the scheduler can make better decisions. I don't actually have any idea whether libuv and rayon would end up in a resource tug-of-war in practice, but it seems like something that would be worth testing...

@kjvalencik
Copy link
Author

Thanks for the feedback @samizdatco! That's some good points about thread pools. Switching to keep using rayon is a little more boilerplate, but I think still an ergonomics win.

let channel = cx.channel();
let (deferred, promise) = cx.promise();

rayon::spawn(move || {
    /* ... */

    channel.settle_with(deferred, |cx| { /* ... */ });
});

Ok(promise)

I'll hold making that change for now because the API is being bikeshed a bit. A couple of questions:

  • Would you expect a panic to automatically reject the promise?
  • When sending a Deferred across a channel to be settled (resolved/rejected) on the main thread, which of these is more clear?
    channel.settle_with(deferred, |cx| { /* ... */ });
    
    // or
    
    deferred.settle_with(&channel, |cx| { /* ... */ });

samizdatco added a commit that referenced this pull request Jun 1, 2022
- dropped the channel-api & rayon::spawn approach
- saveAs and toBuffer now return the promise generated by neon rather than needing an EventEmitter hack
- thanks to @kjvalencik for providing the reference implementation in #40
samizdatco added a commit that referenced this pull request Jul 17, 2022
commit c6a7420
Author: Christian Swinehart <[email protected]>
Date:   Fri Jul 15 17:56:16 2022 -0400

    add bugfix

commit 1019bd5
Author: Christian Swinehart <[email protected]>
Date:   Fri Jul 15 17:53:00 2022 -0400

    update dependencies

commit e89c6df
Author: Christian Swinehart <[email protected]>
Date:   Fri Jul 15 17:46:24 2022 -0400

    recreate the `FontCollection` with every modification

    - previously, the asset-manager was replaced but the parent FontCollection persisted across calls to .use and .reset
    - apparently it was still caching old fonts rather than querying the asset manager each time
    - fixes #94

commit 2a75801
Author: Christian Swinehart <[email protected]>
Date:   Wed Jun 22 16:44:29 2022 -0400

    note glibc 2.28 requirement

commit b6672c3
Author: Christian Swinehart <[email protected]>
Date:   Wed Jun 22 16:43:14 2022 -0400

    pull python2 from older distribution

commit 0ecc7f3
Author: Christian Swinehart <[email protected]>
Date:   Tue Jun 7 18:21:31 2022 -0400

    version 0.9.30

commit a21215a
Author: Christian Swinehart <[email protected]>
Date:   Tue Jun 7 16:30:09 2022 -0400

    Reverting to S3

commit 9eb33a3
Author: Christian Swinehart <[email protected]>
Date:   Tue Jun 7 15:50:40 2022 -0400

    replace s3 in arm-mac build

commit d605629
Author: Christian Swinehart <[email protected]>
Date:   Tue Jun 7 15:34:33 2022 -0400

    post binaries to release rather than s3

commit 5f6ee5f
Author: Christian Swinehart <[email protected]>
Date:   Tue Jun 7 13:19:38 2022 -0400

    build-system updates

    - added support for arm64 on Alpine (fixes #95)
    - debian-stretch no longer supports clang through backports, so moved to buster
    - alpine builds now just pull gn & ninja from the edge distro, leaving libstdc++ compatible with what's on the node:alpine docker image (fixes #97)
    - the glibc & musl images are now multiplatform, so if arm-based runners are ever a thing they'll be ready…
    - self-hosted musl/arm64 builds can’t use js-based workflow actions (as of june 2022) so the arm64 build is all shell for now

commit 735e847
Author: Christian Swinehart <[email protected]>
Date:   Sun Jun 5 16:03:08 2022 -0400

    empty line-breaks preserve spacing

    - addresses the other part of #96

commit 95c030e
Author: Christian Swinehart <[email protected]>
Date:   Sun Jun 5 14:59:20 2022 -0400

    fix baseline calculation for multiline text

    - note the `y` and `baseline` values in a given TextMetrics.lines[] object are independent of one another—they're both relative to the origin of the eventual ctx.fillText() call

commit 3684aca
Author: Christian Swinehart <[email protected]>
Date:   Sun Jun 5 14:00:21 2022 -0400

    fix character range off-by-one error

    - triggered by calling measureText with multiple `\n`s in a row (leading to zero-width lines of typesetting that still consumed one character from the string (see #96)

commit 53c1941
Author: Christian Swinehart <[email protected]>
Date:   Sun Jun 5 12:10:16 2022 -0400

    update distro details

commit 8451ab9
Author: Christian Swinehart <[email protected]>
Date:   Sun Jun 5 12:10:03 2022 -0400

    fix spurious rect exports

commit 3e002e6
Author: Christian Swinehart <[email protected]>
Date:   Sun Jun 5 12:00:24 2022 -0400

    only fill the background if `matte` is set

    - prevents an empty <rect/> at the beginning of every svg export
    - fixes #91

commit 8904658
Author: Christian Swinehart <[email protected]>
Date:   Thu Jun 2 18:14:13 2022 -0400

    note loadImage & FontLibrary updates

commit b1d8b77
Author: Christian Swinehart <[email protected]>
Date:   Thu Jun 2 16:53:32 2022 -0400

    add workaround for libstdc++ version on alpine

commit 10cdb86
Author: Christian Swinehart <[email protected]>
Date:   Thu Jun 2 14:42:35 2022 -0400

    allow previously added fonts to be replaced

    - may address the patched-fonts use case in #94

commit 9c929a7
Author: Christian Swinehart <[email protected]>
Date:   Thu Jun 2 14:40:48 2022 -0400

    add reset() method to FontLibrary

    - removes any local fonts added with the use() method
    - requested in #94

commit e412842
Author: Christian Swinehart <[email protected]>
Date:   Thu Jun 2 11:28:47 2022 -0400

    glob no longer accepts backslash paths for windows

commit da6baf0
Author: Christian Swinehart <[email protected]>
Date:   Wed Jun 1 16:35:32 2022 -0400

    fix loadImage for browsers

    - closes #92

commit bc8cbed
Author: Christian Swinehart <[email protected]>
Date:   Wed Jun 1 16:22:47 2022 -0400

    update to neon 0.10 – use promises & tasks

    - dropped the channel-api & rayon::spawn approach
    - saveAs and toBuffer now return the promise generated by neon rather than needing an EventEmitter hack
    - thanks to @kjvalencik for providing the reference implementation in #40

commit 3606660
Author: Christian Swinehart <[email protected]>
Date:   Wed Jun 1 15:30:35 2022 -0400

    update to neon 0.10 – use the NeonResult type

commit 4f7121c
Author: Christian Swinehart <[email protected]>
Date:   Wed Jun 1 15:25:12 2022 -0400

    update to neon 0.10 – downcasting properties

    - the newly generic `.get` method handles the downcast_or_throw step for us

commit b9d6362
Author: Christian Swinehart <[email protected]>
Date:   Wed Jun 1 15:21:06 2022 -0400

    update to neon 0.10 – buffer access

    - no longer has to happen in a borrow closure

commit 33a7c49
Author: Christian Swinehart <[email protected]>
Date:   Wed Jun 1 15:19:47 2022 -0400

    update neon to 0.10

commit bd77ede
Author: Christian Swinehart <[email protected]>
Date:   Wed Jun 1 13:06:22 2022 -0400

    update dependencies
@samizdatco
Copy link
Owner

Closing this after 2 years of happily using the suggested 1.0 changes

@samizdatco samizdatco closed this Oct 25, 2024
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