Next API WG meeting (held Jul 14 2016)

[mhdawson]

Who

@nodejs/api

When

Meeting set for Thursday July 14 at 12 pm EST.

Where

link for participants: https://hangouts.google.com/hangouts/_/hoaevent/AP36tYenublbryPz0-q_Sjl0OZA599uDMCKR69JlkxDTeW-5BqXy2A?eid=100598160817214911030&hl=en&authuser=0

For those who just want to watch:http://www.youtube.com/watch?v=NJZ2TNul9To

Events page: https://plus.google.com/events/cvkrhrj8hfabfuho1tg5kvmeh5o

Google doc for minutes: https://docs.google.com/document/d/1iQD8JsCa-8_YJnpbd8Y2RTW7ELzY3dER8R9pzhC2fxc/edit

Agenda

• Stand up
• Actions from last meeting
• Update on work for ABI stable module API by - @ianwjhalliday, @sampsongao and @mhdawson
• Open issues review

Quick Links

Links to the code shown in the meeting:

• node fork (look at src/node_jsvmapi.h et al)
• node-addon-examples fork converted to use napi (look at abi folder under each example)
• leveldown fork converted to use napi

[mhdawson]

@nodejs/api - please complete http://doodle.com/poll/c435avwnbpdgid8h to help choose a time for the next meeting.

[mhdawson]

FYI for some people not part of the group @Fishrock123, @jasnell, @kkoopa

[mhdawson]

@nodejs/api last call to update your availability in the doodle

[mhdawson]

@nodejs/api @Fishrock123 @jasnell @kkoopa meeting set for Meeting set for Thursday July 14 at 12 pm EST.

[kkoopa]

Unfortunately I cannot make it then, but I will watch the recording afterwards.

[ianwjhalliday]

Updated the issue description above with the following links for quick reference to the code that was presented today.

• node fork (look at src/node_jsvmapi.h et al)
• node-addon-examples fork converted to use napi (look at abi folder under each example)
• leveldown fork converted to use napi

[kkoopa]

The reason NAN uses Isolate::GetCurrent is Node 0.10, which did not support multiple isolates. Passing a dummy isolate around would not have been good, since it would not have done anything. Its goal has always been identical observable behavior across versions. It would be best to make a clean break and get rid of everything older than Node 4.

I did see one problem in the current proposal: conversion from JavaScript values to real types. This procedure may fail. For example, converting nan (not a number) to an integer. The only sensible way of dealing with this is by returning an error code and using an out arg:

NODE_EXTERN int napi_get_value_uint32(napi_env e, napi_value v, uint32_t *r);

With this comes the problem of API uniformity: an out arg is not strictly necessary when going the other way, since a null pointer acts as a sentinel in a reasonable way, provided that you only ever need a single error code. But them the conversion API will be asymmetric.

[kkoopa]

napi_release_persistent leaks memory, since it operates on objects of type v8::Persistent<v8::Value, v8::NonCopyablePersistentTraits>, which do not have kResetInDestructor set, without resetting them.

Regarding ObjectWrap, it is the garbage collector that is responsible for cleaning up instances. For this to work, the persistent handle needs to be made weak.

[mhdawson]

@kkoopa thanks for the comments, will take a look at how we address them.

[mhdawson]

@nodejs/api PR for minutes from meeting: #24

[ianwjhalliday]

Thanks @kkoopa! First thing I ran into when running the benchmarks was what would have been a mysterious memory leak. Adding the Reset() call before deleting the persistent fixed that right up.

[ianwjhalliday]

@kkoopa indeed we know we need to add some sort of error handling pattern to the API. This is something we have ignored for now but we have been figuring the best approach will be error codes. On the point of consistency I think returning an error code from every API is the way to go, and all other return values will be out parameters.

I did see one problem in the current proposal: conversion from JavaScript values to real types. This procedure may fail. For example, converting nan (not a number) to an integer. The only sensible way of dealing with this is by returning an error code and using an out arg:

NODE_EXTERN int napi_get_value_uint32(napi_env e, napi_value v, uint32_t *r);

With this comes the problem of API uniformity: an out arg is not strictly necessary when going the other way, since a null pointer acts as a sentinel in a reasonable way, provided that you only ever need a single error code. But them the conversion API will be asymmetric.

[ianwjhalliday]

@kkoopa this sounds good. I'll make a note of this and look into removing napi_get_current_env.

The reason NAN uses Isolate::GetCurrent is Node 0.10, which did not support multiple isolates. Passing a dummy isolate around would not have been good, since it would not have done anything. Its goal has always been identical observable behavior across versions. It would be best to make a clean break and get rid of everything older than Node 4.

[ianwjhalliday]

@kkoopa good catch on the ObjectWrap persistent handle. Weak references is an area we have not looked at yet. I believe other VMs may not yet have APIs to do this, or worse don't have a way in their design to provide this. For example, DukTape only does strong references via properties on special C only accessible objects. I have no thought much about it though so perhaps there is a clever solution to this problem.

In general though, weak references must be a requirement to prevent reference cycles causing memory leaks.

Regarding ObjectWrap, it is the garbage collector that is responsible for cleaning up instances. For this to work, the persistent handle needs to be made weak.

[kkoopa]

Well, since WeakMap and WeakSet are part of the ES 2015 specification, any conforming VM already has to support weak references in one way or another. If a VM cannot do that, it cannot support Node (or JavaScript), so this should be a non-issue.

On July 19, 2016 1:03:06 AM GMT+03:00, Ian Halliday [email protected] wrote:

@kkoopa good catch on the ObjectWrap persistent handle. Weak
references is an area we have not looked at yet. I believe other VMs
may not yet have APIs to do this, or worse don't have a way in their
design to provide this. For example, DukTape only does strong
references via properties on special C only accessible objects. I have
no thought much about it though so perhaps there is a clever solution
to this problem.

In general though, weak references must be a requirement to prevent
reference cycles causing memory leaks.

---

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#22 (comment)

[Qard]

Do we want to set ES 2015+ as a requirement for conformance? That'd exclude some of the more minimal VMs, which is a bit unfortunate for devs wanting to use node for embedded systems apps.

[pmuellr]

Do we want to set ES 2015+ as a requirement for conformance?

Seems reasonable to consider loosening that. See how bad the API gets :-)

The API would need to return some kind of indication of which features it didn't support, and then have some kind of defined response when you tried to do something which required a feature that wasn't supported.

Will certainly get tricky if we depend of weak refs as part of the API impl, as suggested with ObjectWrap. For that one, perhaps the interface between the API and JS engine would be expected to do "best effort" - keep those woulda-been-weak-refs in a special pool that the API/JS Engine glue would need to deal with, somehow.

What are you thinking would be on here? Weak refs, Promises, ???

BTW, is anyone currently targeting anything other than V8 yet? DukTape seems like a good candidate ...

[ianwjhalliday]

Since this API is focused on native modules I don't think it would need to know anything about ES6. There are few ES6 features that require host knowledge, and of those it would be node core that needs to know about them, not native modules (e.g. promise continuation callback mechanism). For use of ES6 builtins they can be done by accessing them with normal property gets off the global object.

Getting node core to be VM neutral would be a different story, but one about enabling ES6 features when supported by the underlying VM rather than one about complicating node core development due to having to run on multiple VMs.

WeakMaps don't work for emulating weak references, but they do imply that the VM probably has internal support for weak references that could be exposed to support this API.

We haven't started looking at implementing this prototype API on other VMs yet, but it is our intention to do so to gather more data. Chakra is an obvious choice given the NodeChakra project's significant progress towards functional compatibility. SpiderMonkey is also a reasonable choice given the SpiderNode project.

DukTape would be very interesting since it provides a very different API from the other engines. I am not aware though of any current project trying to build node core on top of DukTape, so this might be too big a task to take on for research. Alternatively it might be feasible to create a small host to standin as a mock node core that provides this same API and implement it with DukTape. Wouldn't be able to easily test this on existing native modules, but could write small test scripts to exercise the design of the prototype API.

[mhdawson]

Minutes landed here: #24.

Going to close this issue (since its scope was the next meeting) , but please open a new issue to continue discussion, or feel free to continue adding comments if they are related to the meeting/content which much of this was.