Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Investigate if canvas can be ported over to use NAPI #41

Closed
aruneshchandra opened this issue Jan 6, 2017 · 5 comments
Closed

Investigate if canvas can be ported over to use NAPI #41

aruneshchandra opened this issue Jan 6, 2017 · 5 comments
Assignees
Milestone

Comments

@aruneshchandra
Copy link
Contributor

At VM Summit 2 - some concern was raised if the current NAPI design can accommodate porting canvas.

@jasongin
Copy link
Member

I've looked into this somewhat. The canvas package appears to be a fairly typical use of NAN, and porting to NAPI should be mostly straightforward. I see just two issues to consider:

  1. The canvas package requires property accessor support, which is not yet implemented in NAPI. I opened a new issue about property accessors.
  2. The canvas code links to some external dependencies (cairo, pango, libjpeg) that must be installed separately. But, I think this does not really affect NAPI at all. The native APIs imported from those external dependencies obviously would not be covered by NAPI.

I'll work more on porting the canvas package to NAPI after the accessor support is implemented, since there's not much point in trying to do it without accessors.

@jasongin
Copy link
Member

I completed a first pass of porting the canvas module to NAPI. Canvas is a a moderately-complex
native module that ends up covering nearly all of the NAPI API surface, so it makes for a good test
of NAPI. Check my fork to browse the differences.

For this purpose I put together a build of NAPI that includes the property accessors and instanceof changes that are in review, since those APIs are heavily used by the canvas code.

Following is a list of additional issues I encountered during the porting effort:

  • Try/Catch (known issue): The NAPI try/catch APIs currently don't offer any way to re-throw an exception after it has been caught.

  • Typed arrays (opened new issue): Canvas exposes image data as UInt8 typed arrays. NAPI currently lacks any support for typed arrays, and as far as I know there is no (compatible, performant) workaround available.

  • AdjustExternalMemory: Canvas calls this API in a few places to give some hints to the V8 GC. It's not critical for the functionality, but I assume it improves GC behavior significantly when working with large images. Unfortunately this is engine-specific; JSRT does not appear to have an equivalent API.

  • Range errors: NAPI needs an easy way to construct/throw the standard JS RangeError type, in addition to the APIs it currently has for TypeError and plain Error.

  • (Lack of) C++ helpers: Code that is converted from V8's C++ classes to NAPI's C function  calls is often a little more verbose, e.g. napi_get_number_from_value(env, v) vs  v->NumberValue(). This could be cleaned up somewhat by adding low/zero-overhead C++ helper classes for NAPI.

So far 65 of 79 canvas unit tests are passing, as well as a majority of the visual tests. The failing tests are mostly due to a few blocks of commented-out in the port dealing with typed arrays.

@gabrielschulhof
Copy link
Collaborator

  • AdjustExternalMemory: Canvas calls this API in a few places to give some hints to the V8 GC. It's not critical for the functionality, but I assume it improves GC behavior significantly when working with large images. Unfortunately this is engine-specific; JSRT does not appear to have an equivalent API.

This could be made a no-op on other implementations, especially since it's not of existential importance.

@jasongin
Copy link
Member

I opened new issues for AdjustExternalMemory and C++ helper classes.

I think I'll just submit some PRs for napi_throw_range_error, as it should be trivial.

I'm closing this issue now since we consider the investigation to be completed.

@Globik
Copy link

Globik commented Jan 5, 2018

Are the async functions in use? No crashes?

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

No branches or pull requests

4 participants