http2: only schedule write when necessary#17183

addaleax · 2017-11-21T11:59:28Z

src: add optional keep-alive object to SetImmediate
Adds the possibility to keep a strong persistent reference to
a JS object while a SetImmediate() call is in effect.
http2: don't call into JS from GC
Calling into JS land from GC is not allowed, so delay
the resolution of pending pings when a session is destroyed.
http2: only schedule write when necessary
Introduce an Http2Scope class that, when it goes out of scope,
checks whether a write to the network is desired by nghttp2.
If that is the case, schedule a write using SetImmediate()
rather than a custom per-session libuv handle.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

http2

Adds the possibility to keep a strong persistent reference to a JS object while a `SetImmediate()` call is in effect.

Calling into JS land from GC is not allowed, so delay the resolution of pending pings when a session is destroyed.

addaleax · 2017-11-21T12:00:45Z

CI: https://ci.nodejs.org/job/node-test-commit/14196/

@nodejs/http2

Introduce an `Http2Scope` class that, when it goes out of scope, checks whether a write to the network is desired by nghttp2. If that is the case, schedule a write using `SetImmediate()` rather than a custom per-session libuv handle.

jasnell · 2017-11-21T16:20:19Z

src/env.h


 typedefvoid (*native_immediate_callback)(Environment* env, void* data);
-inlinevoidSetImmediate(native_immediate_callback cb, void* data);
+// cb will be called as cb(env, data) on the next event lopo iteration.


s/lopo/loop

jasnell

🎉 lgtm... looks like this is yielding a modest perf improvement in simple benchmarks. Very nice.

TimothyGu

Can't comment on anything else really.

TimothyGu · 2017-11-21T17:14:10Z

src/env.h

 structNativeImmediateCallback{
 native_immediate_callback cb_;
 void* data_;
+ v8::Persistent<v8::Object>* keep_alive_;


unique_ptr?

addaleax · 2017-11-27T01:05:02Z

Landed in bb44626...10d86d2

Adds the possibility to keep a strong persistent reference to a JS object while a `SetImmediate()` call is in effect. PR-URL: #17183 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

Calling into JS land from GC is not allowed, so delay the resolution of pending pings when a session is destroyed. PR-URL: #17183 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

Introduce an `Http2Scope` class that, when it goes out of scope, checks whether a write to the network is desired by nghttp2. If that is the case, schedule a write using `SetImmediate()` rather than a custom per-session libuv handle. PR-URL: #17183 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

jasnell · 2017-11-27T17:32:15Z

@addaleax ... since this landed, we've started seeing consistent failures with test-http2-client-upload on windows ... see https://ci.nodejs.org/job/node-test-binary-windows/13154/COMPILED_BY=vs2015,RUNNER=win2008r2-vs2017,RUN_SUBSET=3/ for example.

Can you take a look?

(it might not be this change, but the timing lines up so I'm starting here)

addaleax · 2017-11-27T19:38:24Z

@jasnell I couldn’t reproduce this on Windows or Linux :( Currently running a stress test to see how flaky it is but it’s probably not trivial to figure out why that’s happening…

jasnell · 2017-11-27T19:41:37Z

There has been some flakiness in the past here based on timing of the event loop I/o. I'm trying to recreate locally in Windows now

jasnell · 2017-11-27T19:44:31Z

Yep, unable to recreate locally on win10.

MylesBorins · 2017-12-11T22:28:06Z

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

Fwiw this is not landing cleanly

Introduce an `Http2Scope` class that, when it goes out of scope, checks whether a write to the network is desired by nghttp2. If that is the case, schedule a write using `SetImmediate()` rather than a custom per-session libuv handle. PR-URL: #17183 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

Introduce an `Http2Scope` class that, when it goes out of scope, checks whether a write to the network is desired by nghttp2. If that is the case, schedule a write using `SetImmediate()` rather than a custom per-session libuv handle. Backport-PR-URL: #18050 PR-URL: #17183 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

Adds the possibility to keep a strong persistent reference to a JS object while a `SetImmediate()` call is in effect. Backport-PR-URL: #18050 PR-URL: #17183 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

Calling into JS land from GC is not allowed, so delay the resolution of pending pings when a session is destroyed. Backport-PR-URL: #18050 PR-URL: #17183 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

Introduce an `Http2Scope` class that, when it goes out of scope, checks whether a write to the network is desired by nghttp2. If that is the case, schedule a write using `SetImmediate()` rather than a custom per-session libuv handle. Backport-PR-URL: #18050 PR-URL: #17183 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

MylesBorins · 2018-01-24T09:25:10Z

assuming this should be included in a larger http2 backport for 8.x

Adds the possibility to keep a strong persistent reference to a JS object while a `SetImmediate()` call is in effect. PR-URL: nodejs#17183 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

Calling into JS land from GC is not allowed, so delay the resolution of pending pings when a session is destroyed. PR-URL: nodejs#17183 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

Introduce an `Http2Scope` class that, when it goes out of scope, checks whether a write to the network is desired by nghttp2. If that is the case, schedule a write using `SetImmediate()` rather than a custom per-session libuv handle. Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17183 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

Adds the possibility to keep a strong persistent reference to a JS object while a `SetImmediate()` call is in effect. Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17183 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

Calling into JS land from GC is not allowed, so delay the resolution of pending pings when a session is destroyed. Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17183 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

Introduce an `Http2Scope` class that, when it goes out of scope, checks whether a write to the network is desired by nghttp2. If that is the case, schedule a write using `SetImmediate()` rather than a custom per-session libuv handle. Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17183 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

Adds the possibility to keep a strong persistent reference to a JS object while a `SetImmediate()` call is in effect. Backport-PR-URL: #18050 Backport-PR-URL: #20456 PR-URL: #17183 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

Calling into JS land from GC is not allowed, so delay the resolution of pending pings when a session is destroyed. Backport-PR-URL: #18050 Backport-PR-URL: #20456 PR-URL: #17183 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

Introduce an `Http2Scope` class that, when it goes out of scope, checks whether a write to the network is desired by nghttp2. If that is the case, schedule a write using `SetImmediate()` rather than a custom per-session libuv handle. Backport-PR-URL: #18050 Backport-PR-URL: #20456 PR-URL: #17183 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

addaleax added 2 commits November 21, 2017 12:51

src: add optional keep-alive object to SetImmediate
d5033aa
Adds the possibility to keep a strong persistent reference to a JS object while a `SetImmediate()` call is in effect.

http2: don't call into JS from GC
1e3b395
Calling into JS land from GC is not allowed, so delay the resolution of pending pings when a session is destroyed.

addaleax added the http2 Issues or PRs related to the http2 subsystem. label Nov 21, 2017

addaleax requested a review from jasnell November 21, 2017 11:59

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 21, 2017

http2: only schedule write when necessary
0e14510
Introduce an `Http2Scope` class that, when it goes out of scope, checks whether a write to the network is desired by nghttp2. If that is the case, schedule a write using `SetImmediate()` rather than a custom per-session libuv handle.

addaleax force-pushed the http2-setimmediate branch from b215a8b to 0e14510Compare November 21, 2017 12:55

jasnell reviewed Nov 21, 2017
View reviewed changes

jasnell approved these changes Nov 21, 2017
View reviewed changes

TimothyGu reviewed Nov 21, 2017
View reviewed changes

[squash] jasnell & TimothyGu comments
02c7022

fhinkel approved these changes Nov 21, 2017
View reviewed changes

apapirovski approved these changes Nov 21, 2017
View reviewed changes

addaleax closed this Nov 27, 2017

addaleax deleted the http2-setimmediate branch November 27, 2017 01:05

apapirovski mentioned this pull request Dec 8, 2017
v8.9.3 proposal #17532
Merged

MylesBorins added the backport-requested-v9.x label Dec 11, 2017

MylesBorins mentioned this pull request Jan 10, 2018
v9.4.0 proposal #18069
Merged

addaleax added backported-to-v9.x and removed backport-requested-v9.x labels Jan 10, 2018

MylesBorins added dont-land-on-v6.x labels Jan 19, 2018

MylesBorins mentioned this pull request May 2, 2018
v8.11.2 proposal #20478
Merged

BethGriggs removed the lts-watch-v8.x label Sep 19, 2019

Uh oh!

http2: only schedule write when necessary#17183

http2: only schedule write when necessary #17183

Uh oh!

Conversation

addaleax commented Nov 21, 2017

Checklist

Affected core subsystem(s)

Uh oh!

addaleax commented Nov 21, 2017

Uh oh!

jasnellNov 21, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

TimothyGuNov 21, 2017

Choose a reason for hiding this comment

Uh oh!

addaleax commented Nov 27, 2017

Uh oh!

jasnell commented Nov 27, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Nov 27, 2017

Uh oh!

jasnell commented Nov 27, 2017

Uh oh!

jasnell commented Nov 27, 2017

Uh oh!

MylesBorins commented Dec 11, 2017

Uh oh!

MylesBorins commented Jan 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

jasnell commented Nov 27, 2017•
edited
Loading