Repository navigation
Node Foundation TSC Meeting 2015-09-02 #2654
Description
Activity
Would love it if we could include a quick discussion on nominating new TSC members.
(At least just to pull together a short list, not necessarily to decide on today)
A summary for the TS of Inspecting Node.js with Chrome DevTools #2546 would be appreciated from anyone involved in there, particularly what is it that the TSC needs to care about and are we being called upon to make decisions here. Preferably this should be given here before the meeting so we don't end up wasting all our time trying to grok it all.
I tagged deps: update v8 to 4.5.103.30 #2632 because there is an outstanding question of whether we want to try and slip V8 4.5 into v4 since it (surprisingly) went stable today/yesterday. There's pros and cons to this. Pros: we get a supported V8 for longer, we get some bugs fixed and some new features like arrow functions (a weak justification but I like how this adds to the "upgrade to v4" story that we need to be pushing), possibly some perf improvements (including a potential backport or @indutny's patch that just landed there). Cons: Uncertain at this stage if NAN supports this version, it was supposed to but it looks like the V8 team may have slipped a breaking API change in that wasn't pre-announced, there's also some potential pain for addon authors who may think they are ready when in fact they are not since there has been a full
s/Handle/Localand although we knew about this, it wasn't widely known to addon authors who could get away with liberalHandleusage. Also cons: limited testing, while we've had v3.x using 4.4 for a while now.TSC expansion discussion absolutely, I started an email thread about that but we should use some of the private time at the begining of the meeting to talk openly about it, I'd love to see us bring in more of the quality collaborators we have and get some more diverse perspectives and skills!
it looks like the V8 team may have slipped a breaking API change in that wasn't pre-announced
It would be good if this could be made a bit more concrete. Based on the thread it looks like this is referring to
Local<Value> v8::Function::Call()being markedV8_DEPRECATE_SOON, which is two steps away from an actual breaking change (first, move toV8_DEPRECATE; second, actually remove).it wasn't widely known to addon authors who could get away with liberal Handle usage.
Just want to point out that this should have been giving deprecation warnings, so there's at least some chance they'd be aware. Although there's no guarantee addon authors have been compiling with new enough V8, so I understand the general point.
@domenic sorry, it's not
Function::Call(), I was talking aboutFunction::New()but looking again it and seeing the full section of the code it seems that this might be what @trevnorris was talking about too.- static Local<Function> New(Isolate* isolate, - FunctionCallback callback, - Local<Value> data = Local<Value>(), - int length = 0); + static MaybeLocal<Function> New(Local<Context> context, + FunctionCallback callback, + Local<Value> data = Local<Value>(), + int length = 0); + static V8_DEPRECATE_SOON( + "Use maybe version", + Local<Function> New(Isolate* isolate, FunctionCallback callback, + Local<Value> data = Local<Value>(), int length = 0));
Unfortunately I haven't been able to do any testing on this, no time at all. If anyone else has time it'd be great to get some testing of NAN on this version. @kkoopa do you have time to verify the NAN test suite against #2632 on top of
vee-eight-4.5?it wasn't widely known to addon authors who could get away with liberal Handle usage.
Handle is defined like this in the header file:
#if !defined(V8_IMMINENT_DEPRECATION_WARNINGS) // Local is an alias for Local for historical reasons. template <class T> using Handle = Local<T>; #endif
If I understand it well, as long as we don't enable
V8_IMMINENT_DEPRECATION_WARNINGSin v4, addons using it will still work.@targos my point is about all of the core APIs now exporting
Localinstead ofHandle, but perhaps that's moot since it should just be an alias. Again, some testing would be nice. Those who would like to see 4.5 in should be out there testing addons and seeing what kind of work is required. I'd start by looking at buffertools, bignum, leveldown (ones that the TSC has some connection with), also interesting would be anything in npm that's depending on nan@2.x if you can figure that out. If we can get a level of confidence that there's not going to be much/any pain from this then it's more likely this will make it in.@rvagg here is a list of all npm packages where the nan dependency satisfies 2.0.8:
buffer-indexof-fast ed25519-supercop gigecamera-simulator ifx_db iobroker.terminal linux-ns lru-addon magickwand mmap-io node-shared-cache nodeos-mount qrencode-raw sd-daemon sdb sypexgeo-nan usage-nan wocr xenstat zlib-raw-sync bignum blake2 forsake imageplus memwatch-next mitie nsfw snappy sse4_crc32 xz fsevents blackbone-node png-img utun v8-debug addon-emitter bluetooth-hci-socket bufferutil capture-window edgeswim-uvmon farmhash fds fs-xattr function-origin gc-stats gdx hackrf hiredis inotify macos-alias modern-syslog naio-test natrium node-minizip node-openalpr node-opencv-zbar oid pattimura phash-image sha3 torium utf-8-validate weak webkitgtk xml-conv-json xpc-connection deasync cap freetype2 lodepng mknod mmmagic nbind node-expat node-gpgme node-sass re2 sharp number-smusher npool neventemitter keybase-sodium sodium-prebuilt kbd sleep lzma-native node-libcurl leveldown nodejieba pcsclite traceview-bindings zmq iconv node-ios-device async_bench effective epoll geoip mosca v8-profiler arc4random libxl unix-dgram arrayfire_js electron-ref ref time kinect2 microtime mkfifo runas runas-compiled userinfoAll Function::New() does is call FunctionTemplate::New()->GetFunction(). It is probably not even in common use, as it was not available before 0.12.
There are four ways of fixing it:
- Patch V8 to remove the offending commit
- Patch NAN and keep the existing behavior
- Patch NAN and update to the new behavior
- Patch NAN and remove it completely.
Witchever the resolution is, I strongly oppose bumping the major version of NAN for this uselessness. 2.x is here to stay for at year, breakage be damned.
The Handle stuff is a non-issue. It is trivial to fix, it has been known for over two years that it needs to be fixed, and there is no promise of forward compatibility. It was well known already before the release of io.js 3.0.
On September 2, 2015 4:07:59 PM GMT+03:00, Rod Vagg notifications@github.com wrote:
@domenic sorry, it's not
Function::Call(), I was talking about
Function::New()but looking again it and seeing the full section of
the code it seems that this might be what @trevnorris was talking about
too.- static Local<Function> New(Isolate* isolate, - FunctionCallback callback, - Local<Value> data = Local<Value>(), - int length = 0); + static MaybeLocal<Function> New(Local<Context> context, + FunctionCallback callback, + Local<Value> data = Local<Value>(), + int length = 0); + static V8_DEPRECATE_SOON( + "Use maybe version", + Local<Function> New(Isolate* isolate, FunctionCallback callback, + Local<Value> data = Local<Value>(), int length = 0));
Unfortunately I haven't been able to do any testing on this, no time at
all. If anyone else has time it'd be great to get some testing of NAN
on this version. @kkoopa do you have time to verify the NAN test suite
against #2632 on top ofvee-eight-4.5?
Reply to this email directly or view it on GitHub:
#2654 (comment)Sorry, I can't make it today.
Most likely I won't be able to attend today.
@kkoopa so my understanding of your four ways to fix it is that you would find patching NAN to keep the existing behavior or update to the new behavior both acceptable? Neither would require a major version bump, correct?
Anyway, again it sounds like this is not actually a removal, or even a deprecation, just a deprecation-soon. So NAN doesn't need to do anything at all, if it doesn't want to, at least for two more V8 releases. As @rvagg says though, getting some testing in would be crucial to confirm.
3 remaining items
The DevTools team has engaged us about separating v8-inspector from Blink and making a package that we could use. This would allow anyone to open Chrome, browse to
chrome://inspectand point it at a node process. A few points from the discussion are:- The remote debugger API (that we'd consume) has remained fairly stable.
- If it does break, it's still possible to load an old Chrome DevTools console to consume the older API. So this should continue to work in an LTS release.
- From the thread: "On the DevTools side we’ll be providing generic API that would work for both Blink and Node.js".
- The API will integrate pretty tightly with our existing code in order to collect all the information it needs. e.g. asynchronous stacks.
- Node would have to fully support web sockets in order to communicate with Chrome.
- Node would have to run another thread that also executes JS. Though I don't believe it would need the full event loop.
My main concern are the integration points. While many of them can be done in native, which makes the conditional practically a performance noop, we will have to add a conditional in
nextTick(). Which performance has already degraded since additional logic has been added.@paulirish listed some of the features that would be included: #2546 (comment)
I'm unsure of how this would conflict, or if it would conflict, with the work @nodejs/tracing is doing. @ofrobots Do you have any feedback here?
I can't make it today, let me know if there is anything you need from the
foundation and i'll take care of it.On Wednesday, September 2, 2015, Trevor Norris notifications@github.com
wrote:@rvagg https://github.com/rvagg Summary for #2546
#2546:The DevTools team has engaged us about separating v8-inspector from Blink
and making a package that we could use. This would allow anyone to open
Chrome, browse to chrome://inspect and point it at a node process. A few
points from the discussion are:- The remote debugger API (that we'd consume) has remained fairly
stable. - If it does break, it's still possible to load an old Chrome DevTools
console to consume the older API. So this should continue to work in an LTS
release. - From the thread: "On the DevTools side we’ll be providing generic
API that would work for both Blink and Node.js". - The API will integrate pretty tightly with our existing code in
order to collect all the information it needs. e.g. asynchronous stacks. - Node would have to fully support web sockets in order to communicate
with Chrome. - Node would have to run another thread that also executes JS. Though
I don't believe it would need the full event loop.
My main concern are the integration points. While many of them can be done
in native, which makes the conditional practically a performance noop, we
will have to add a conditional in nextTick(). Which performance has
already degraded since additional logic has been added.@paulirish https://github.com/paulirish listed some of the features
that would be included: #2546 (comment)
#2546 (comment)I'm unsure of how this would conflict, or if it would conflict, with the
work @nodejs/tracing https://github.com/orgs/nodejs/teams/tracing is
doing. @ofrobots https://github.com/ofrobots Do you have any feedback
here?—
Reply to this email directly or view it on GitHub
#2654 (comment).- The remote debugger API (that we'd consume) has remained fairly
@trevnorris I don't see much conflict between v8-inspector and trace-event. Blink already uses both trace-event and DevTools.
The TSC call was yesterday? 😕
@mscdex sorry, dates are hard .. I always have to wind back by a day when I do these cause I'm a day ahead, I obviously wound back by two!
@jkrems: yes, we've switched to being more timely now we have everything in place, the last minutes were merged with a soundcloud link in it and I'm going to try and be quicker in future in putting in the PRs. If you look in the doc/tsc-minutes/ directory you'll see SoundCloud links on all of those.
@rvagg The thing is - the meeting issues are easy to find (for me at least) but even after you told me that the meeting notes were merged with the soundcloud link, I wasn't able to find them (without cheating and using the exact directory path in your comment). E.g. Google only leads to https://nodejs.org/en/foundation/tsc/minutes/. I think just adding a short "Meetings notes will be made available in
doc/tsc-minutesand audio recordings on SoundCloud." at the end of the issue template would go a long way. Just to make it easier to discover for people outside the loop. :)Was confirmed that there's no need to add async stack trace support from the beginning: #2546 (comment)
@jkrems I'll try and keep that in mind for next week, feel free to hassle me if I've forgotten tho!
- changed the title
[-]Node Foundation TSC Meeting 2015-09-01[/-][+]Node Foundation TSC Meeting 2015-09-02[/+]on Sep 3, 2015 https://soundcloud.com/node-foundation/tsc-meeting-2015-09-02 is audio for this meeting, PRing in the minutes now
I'm unsure of how this would conflict, or if it would conflict, with the work @nodejs/tracing is doing.
Note that we already expose tracing over the remote debugging protocol. DevTools Timeline panel is implemented based on that. If it makes sense for Node.js too we can consider adding it to v8-inspector. The protocol implementation for tracing domain is fairly simple.
@rvagg Can this be closed now?
Time
UTC Wed 02-Sep-15 20:00:
Or in your local time:
Links
Agenda
Extracted from tsc-agenda labelled issues and pull requests from the nodejs org prior to meeting.
nodejs/node
nodejs/build
Invited
Notes
The agenda comes from issues labelled with
tsc-agendaacross all of the repositories in the nodejs org. Please label any additional issues that should be on the agenda before the meeting starts. I'm using a tool to fetch the list so it's not a big deal to collate.Joining the meeting
Uberconference; participants should have the link & numbers, contact me if you don't.