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

Node Foundation TSC Meeting 2015-09-02 #2654

Closed
rvagg opened this issue Sep 2, 2015 · 29 comments
Closed

Node Foundation TSC Meeting 2015-09-02 #2654

rvagg opened this issue Sep 2, 2015 · 29 comments
Labels
meta Issues and PRs related to the general management of the project.

Comments

@rvagg
Copy link
Member

rvagg commented Sep 2, 2015

Time

UTC Wed 02-Sep-15 20:00:

  • San Francisco: Wed 02-Sep-15 13:00
  • Amsterdam: Wed 02-Sep-15 22:00
  • Moscow: Wed 02-Sep-15 23:00
  • Sydney: Thu 03-Sep-15 06:00
  • Tokyo: Thu 03-Sep-15 05: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

  • deps: update v8 to 4.5.103.30 #2632
  • Inspecting Node.js with Chrome DevTools #2546
  • Node.js v4 Release Timeline #2522

nodejs/build

  • Merge job overwrites metadata #179

Invited

Notes

The agenda comes from issues labelled with tsc-agenda across 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.

@jasnell
Copy link
Member

jasnell commented Sep 2, 2015

Would love it if we could include a quick discussion on nominating new TSC members.

@jasnell
Copy link
Member

jasnell commented Sep 2, 2015

(At least just to pull together a short list, not necessarily to decide on today)

@rvagg
Copy link
Member Author

rvagg commented Sep 2, 2015

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/Local and although we knew about this, it wasn't widely known to addon authors who could get away with liberal Handle usage. Also cons: limited testing, while we've had v3.x using 4.4 for a while now.

@rvagg
Copy link
Member Author

rvagg commented Sep 2, 2015

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!

@domenic
Copy link
Contributor

domenic commented Sep 2, 2015

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 marked V8_DEPRECATE_SOON, which is two steps away from an actual breaking change (first, move to V8_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.

@rvagg
Copy link
Member Author

rvagg commented Sep 2, 2015

@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 of vee-eight-4.5?

@targos
Copy link
Member

targos commented Sep 2, 2015

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_WARNINGS in v4, addons using it will still work.

@rvagg
Copy link
Member Author

rvagg commented Sep 2, 2015

@targos my point is about all of the core APIs now exporting Local instead of Handle, 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 [email protected] 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.

@targos
Copy link
Member

targos commented Sep 2, 2015

@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
userinfo

@kkoopa
Copy link

kkoopa commented Sep 2, 2015

All 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 [email protected] 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 of vee-eight-4.5?


Reply to this email directly or view it on GitHub:
#2654 (comment)

@shigeki
Copy link
Contributor

shigeki commented Sep 2, 2015

Sorry, I can't make it today.

@orangemocha
Copy link
Contributor

Most likely I won't be able to attend today.

@Fishrock123 Fishrock123 added the meta Issues and PRs related to the general management of the project. label Sep 2, 2015
@domenic
Copy link
Contributor

domenic commented Sep 2, 2015

@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.

@kkoopa
Copy link

kkoopa commented Sep 2, 2015

@domenic Patching to update to new behavior would be a breaking change, but I still do not consider it significant enough to warrant a major version bump, regardless of semver this or semver that. The construct is probably not used outside of NAN's test suite.

@domenic
Copy link
Contributor

domenic commented Sep 2, 2015

OK. So it sounds like patching NAN to keep the existing behavior is the way to go.

@kkoopa
Copy link

kkoopa commented Sep 2, 2015

It's probably the easiest fix, yes. Will take a minute to cook up.

Personally I would prefer removing the commit from V8, since it was not in 4.3, where it belonged, and I am tired of the haphazard flailing back and forth with breaking changes everywhere from one patch release to another with no concrete plan. But this is mostly just me being obstinate.

On September 2, 2015 5:58:56 PM GMT+03:00, Domenic Denicola [email protected] wrote:

OK. So it sounds like patching NAN to keep the existing behavior is the
way to go.


Reply to this email directly or view it on GitHub:
#2654 (comment)

@jkrems
Copy link
Contributor

jkrems commented Sep 2, 2015

OT/meta: Could the Soundcloud account where the recording of the meeting is uploaded be made part of the meeting notes and/or the announcement issue? I missed the switch and was still looking at the Youtube account wondering why the recordings weren't showing up anymore. Took some googling and dead-ends to find the right place.

@trevnorris
Copy link
Contributor

@rvagg Summary for #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 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?

@mikeal
Copy link
Contributor

mikeal commented Sep 2, 2015

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 [email protected]
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).

@ofrobots
Copy link
Contributor

ofrobots commented Sep 2, 2015

@trevnorris I don't see much conflict between v8-inspector and trace-event. Blink already uses both trace-event and DevTools.

@mscdex
Copy link
Contributor

mscdex commented Sep 2, 2015

The TSC call was yesterday? 😕

@cjihrig
Copy link
Contributor

cjihrig commented Sep 2, 2015

@mscdex I think that's just a typo.

@rvagg
Copy link
Member Author

rvagg commented Sep 2, 2015

@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.

@jkrems
Copy link
Contributor

jkrems commented Sep 2, 2015

@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-minutes and 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. :)

@trevnorris
Copy link
Contributor

Was confirmed that there's no need to add async stack trace support from the beginning: #2546 (comment)

@rvagg
Copy link
Member Author

rvagg commented Sep 3, 2015

@jkrems I'll try and keep that in mind for next week, feel free to hassle me if I've forgotten tho!

@rvagg rvagg changed the title Node Foundation TSC Meeting 2015-09-01 Node Foundation TSC Meeting 2015-09-02 Sep 3, 2015
@rvagg
Copy link
Member Author

rvagg commented Sep 3, 2015

https://soundcloud.com/node-foundation/tsc-meeting-2015-09-02 is audio for this meeting, PRing in the minutes now

@yury-s
Copy link

yury-s commented Sep 3, 2015

@trevnorris

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.

@thefourtheye
Copy link
Contributor

@rvagg Can this be closed now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

No branches or pull requests