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

http2: only schedule write when necessary #17183

Closed
wants to merge 4 commits into from

Conversation

addaleax
Copy link
Member

  • 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 addaleax added the http2 Issues or PRs related to the http2 subsystem. label Nov 21, 2017
@nodejs-github-bot 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
@addaleax
Copy link
Member Author

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.
src/env.h Outdated
@@ -686,7 +686,11 @@ class Environment {
bool EmitNapiWarning();

typedef void (*native_immediate_callback)(Environment* env, void* data);
inline void SetImmediate(native_immediate_callback cb, void* data);
// cb will be called as cb(env, data) on the next event lopo iteration.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/lopo/loop

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't comment on anything else really.

src/env.h Outdated
@@ -751,6 +755,7 @@ class Environment {
struct NativeImmediateCallback {
native_immediate_callback cb_;
void* data_;
v8::Persistent<v8::Object>* keep_alive_;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unique_ptr?

@addaleax
Copy link
Member Author

Landed in bb44626...10d86d2

@addaleax addaleax closed this Nov 27, 2017
@addaleax addaleax deleted the http2-setimmediate branch November 27, 2017 01:05
addaleax added a commit that referenced this pull request Nov 27, 2017
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]>
addaleax added a commit that referenced this pull request Nov 27, 2017
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]>
addaleax added a commit that referenced this pull request Nov 27, 2017
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
Copy link
Member

jasnell commented Nov 27, 2017

@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
Copy link
Member Author

@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
Copy link
Member

jasnell commented Nov 27, 2017

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
Copy link
Member

jasnell commented Nov 27, 2017

Yep, unable to recreate locally on win10.

@apapirovski apapirovski mentioned this pull request Dec 8, 2017
@MylesBorins
Copy link
Contributor

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

MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
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]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
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 MylesBorins mentioned this pull request Jan 10, 2018
MylesBorins pushed a commit that referenced this pull request Jan 10, 2018
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]>
MylesBorins pushed a commit that referenced this pull request Jan 10, 2018
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]>
MylesBorins pushed a commit that referenced this pull request Jan 10, 2018
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
Copy link
Contributor

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

MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Apr 12, 2018
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]>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Apr 12, 2018
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]>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Apr 12, 2018
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]>
kjin pushed a commit to kjin/node that referenced this pull request May 1, 2018
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]>
kjin pushed a commit to kjin/node that referenced this pull request May 1, 2018
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]>
kjin pushed a commit to kjin/node that referenced this pull request May 1, 2018
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]>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
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]>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
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]>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
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]>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues or PRs related to the http2 subsystem. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants