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

Deep dive discussion on {Async,Promise}_hooks & performance #248

Closed
ofrobots opened this issue Oct 17, 2018 · 42 comments
Closed

Deep dive discussion on {Async,Promise}_hooks & performance #248

ofrobots opened this issue Oct 17, 2018 · 42 comments
Labels

Comments

@ofrobots
Copy link
Contributor

During the WG meeting today we discussed that It would be good to meet and unblock the topic of async hooks / promise hooks performance. (#188).

Context:

Here's a doodle: https://doodle.com/poll/fqkzvw929ueys2u9 I want to be able to get at least @hashseed @mcollina @AndreasMadsen to be able to attend. Please mark your availability by Friday Oct 19 and I will get a deep dive discussion scheduled.

@AndreasMadsen
Copy link
Member

I would like @mike-kaufman or another from Microsoft to also attend, as they appear to be the primary driver of "JavaScript Async Context".

@Qard
Copy link
Member

Qard commented Oct 18, 2018

I'd like to attend if I can, so I put my times up. I have some thoughts on the subject. It's okay if it doesn't line up for me though. Definitely @hashseed, @mcollina and @AndreasMadsen are the core people we need most in the conversation.

@ofrobots
Copy link
Contributor Author

It would be great to have as many people attend as possible. Please add your preferences, and I will try my best to find a good timeslot 🤞.

@mcollina
Copy link
Member

mcollina commented Oct 19, 2018 via email

@Flarna
Copy link
Member

Flarna commented Oct 19, 2018

These evening slots are in general hard for me and also difficult to predict. I don't add anything in doodle to avoid to bias it but I will try to attend.

@davisjam
Copy link

Will this conversation be recorded?

@mhdawson
Copy link
Member

I'll try to attend as well, but think it is more important that times are good for other participants so won't complete the doodle.

@ofrobots
Copy link
Contributor Author

ofrobots commented Oct 23, 2018

Based on the poll, the best meeting time slot seems to be Oct 30, 11am PDT (1900 Central Europe) (your local time).

Hangouts meeting link: https://meet.google.com/tov-fcdh-jfw

@AndreasMadsen
Copy link
Member

Is that 18:00 UTC? I'm switching to daylight saving time on the 28th, so I'm a bit worried and would like a fixed timezone specifier.

@wentout
Copy link

wentout commented Oct 24, 2018

I'm very sorry, but I'm very interested in the result of this discussion.
Can I join and listen also?
I closed Issue #249, cause seems is was bit duplicate and everyone knows the idea, described there.

@ofrobots
Copy link
Contributor Author

@AndreasMadsen AFAIK that should be Oct 30th 1800 UTC. Timezones are hard 😂.

@wentout you're welcome to join the hangouts at the time of the meeting!

@mmarchini
Copy link
Contributor

Google Calendar should handle timezones and daylight saving time correctly, at least it does here when I look at the Node.js Foundation Calendar.

@wentout
Copy link

wentout commented Oct 25, 2018

image

So, it made all necessary translations to my local time. Very easy and usable.

@ofrobots
Copy link
Contributor Author

Just a reminder that this meeting will start in about 1-hr's time. It would be useful to review the prior discussions before the meeting (see OP).

At the time of the meeting, participants can join the meeting at: https://meet.google.com/tov-fcdh-jfw.

@mmarchini
Copy link
Contributor

I'm on the road and won't make it :(

@mcollina
Copy link
Member

Here is the link to the domain issue: nodejs/node#23862

@mcollina
Copy link
Member

Here is the link to the currentAsyncResource() latest PR: https://github.com/mcollina/node/tree/current-resource-bis.

@ofrobots
Copy link
Contributor Author

The recording of the meeting is here: https://youtu.be/2SCNsozMkF8. Thanks for everyone for participating.

Here's a brief summary of the action plan:

  • We discussed that it would be great if majority of the async-hooks uses-cases didn't need to use the destroy hook. The hook can stay, but the expectation is that the availability of the resource on hook callbacks will remove the need for APMs to install destroy hooks. @mcollina's PR async_hooks: add currentResource node#21313 needs to be resurrected. Looking for volunteers. If there are no volunteers, @mcollina will pick this up again once he has some free time in December. There needs to be some consideration of whether it would problematic to expose the promise as the resource object. @hashseed to check with the V8 compiler team about gotchas. @ofrobots and @AndreasMadsen to provide concrete examples where we observe promises taking too long to GC.
  • @hashseed to work with Maya on the V8 compiler team to introduce promise hooks from JS. Once these are added, Node core can switch to these.
  • General agreement that non-observable promises don't need to trigger hooks as long as context preserved. V8 team to implement this optimization.
  • V8 team expressed a desire for some spec modification describing promise hooks behavior. General agreement that this would be a good thing to get the interface between V8 and Node solidified. @hashseed to point to specific section and then WG can work drafting some spec test.

Let me know if I missed or misunderstood anything. Full details in the recording.

@mmarchini
Copy link
Contributor

@mcollina's PR nodejs/node#21313 needs to be resurrected. Looking for volunteers. If there are no volunteers

I can do that. Should I just resubmit the same PR or are there any changes required before resubmitting it?

@mcollina
Copy link
Member

@mmarchini there are some comments in the PR that needs to be fixed, mainly there is an ask that we maintain a stack of the current async resources.

@mmarchini
Copy link
Contributor

Ok, thanks. I'll take a look into it next week.

@Flarna
Copy link
Member

Flarna commented Oct 31, 2018

@mcollina Could you please point to the fix regarding http_parser which allows to use the async resource in before/after?

@mcollina
Copy link
Member

mcollina commented Nov 1, 2018

@Flarna here it is nodejs/node#23201.

@AndreasMadsen
Copy link
Member

AndreasMadsen commented Nov 5, 2018

@mcollina That PR looks like it is just a small fix. As far as I can see, the same resource object is still being reused. I would expect a fix that allows WeakMap to be used to involve something with the freelist and the HTTP_PARSER allocator.

@Flarna
Copy link
Member

Flarna commented Nov 5, 2018

I agree with @AndreasMadsen. If a user stores some metadata into the HTTPParser object it will stay alive even after the parser is freed.
Maybe some dummy/wrapper object (similar as for Promises) should be created on every alloc and passed as resource to the callbacks. Doc states nothing really concrete users can rely on about the resource so it should be don't care.

@AndreasMadsen
Copy link
Member

@Flarna to be clear the docs specifically says:

In some cases the resource object is reused for performance reasons, it is thus not safe to use it as a key in a WeakMap or add properties to it.

Once the issue with HTTPParser is fixed, I'm fine with changing that.

@mcollina
Copy link
Member

mcollina commented Nov 6, 2018

@AndreasMadsen I cannot reproduce the problem anymore. Can you share a snippet that reproduces it?

'use strict'

const sleep = require('util').promisify(setTimeout)
const { createHook, executionAsyncResource } = require('async_hooks')
const asyncHook = createHook({init})
const sym = Symbol('cls')

asyncHook.enable()

function getCLS () {
  return executionAsyncResource()[sym]
}

function setCLS (data) {
  return executionAsyncResource()[sym] = data
}

function init (asyncId, type, triggerAsyncId, resource) {
  var cr = executionAsyncResource()
  if (cr) {
    if (type === 'HTTP_PARSER') {
      if (cr.noReuse) {
        throw new Error('no reuse')
      }

      cr.noReuse = true
    }

    resource[sym] = cr[sym]
  }
}


var counter = 0

const server = require('http').createServer(function (req, res) {
  setCLS(counter++)
  sleep(10).then(function () {
    res.setHeader('content-type', 'application/json')
    res.end(JSON.stringify({ cls: getCLS() }))
  })
})
server.listen(3000)

@AndreasMadsen
Copy link
Member

@mcollina I'm not in a position to compile anything right now, but

'use strict'

const http = require('http')
const { createHook } = require('async_hooks')
const reused = Symbol('reused')

const asyncHook = createHook({
  init (asyncId, type, triggerAsyncId, resource) {
    if (resource[reused]) {
      throw new Error('reuse detected')
    }
    resource[reused] = true;
  }
})
asyncHook.enable()

const server = http.createServer(function (req, res) {
  res.end();
})
server.listen(3000, function () {
  http.get('http://127.0.0.1:3000', function () {
    http.get('http://127.0.0.1:3000', function () {
      console.log('passed');
      server.close();
    });
  });
});

is the test you need.

@AndreasMadsen
Copy link
Member

@mcollina tested it now, the issue is still there.

@mmarchini
Copy link
Contributor

Implementing a wrapper on HTTPParser will be tricky 🤔. Do we really need to reuse HTTPParsers though? I changed the max amount of parsers in the FreeList at to 0, which forces Node.js to always create a new parser, and our benchmarks don't show significant regressions:

                                      confidence improvement accuracy (*)   (**)  (***)
 http/bench-parser.js n=100000 len=16                -2.47 %       ±3.05% ±4.07% ±5.34%
 http/bench-parser.js n=100000 len=32                -1.58 %       ±3.74% ±4.98% ±6.48%
 http/bench-parser.js n=100000 len=4                 -0.02 %       ±4.74% ±6.31% ±8.22%
 http/bench-parser.js n=100000 len=8                 -0.94 %       ±3.44% ±4.58% ±5.96%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case there are 4 comparisons, you can thus
expect the following amount of false-positive results:
  0.20 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.04 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

compare-plot

(I might be running the wrong benchmarks though. Are there any other recommended benchmarks I should run?)

@mcollina
Copy link
Member

mcollina commented Nov 12, 2018 via email

@mmarchini
Copy link
Contributor

That's what I was thinking. Also, I don't think there would be any difference between removing the reuse and wrapping HTTPParse (we would need to allocate the wrapper every time anyway).

@Flarna
Copy link
Member

Flarna commented Nov 12, 2018

I think the main difference is that a wrapper object would be a simple javascript object linked to HTTPParser and this link is cut once the parser is moved into the freelist whereas a HTTPParser object has a native part attached so it's more heavy.
But I fully agree that it's even better to avoid the complexity of introducing a wrapper just because of this.

mmarchini added a commit to mmarchini/node that referenced this issue Nov 13, 2018
As discussed in nodejs/diagnostics#248 and
nodejs#21313, reusing HTTPParser resource
is a blocker for landing
`require('async_hooks').currentAsyncResource()`, since reusing an async
resource interanlly would make it infeasible to use them reliably in
WeakMaps. Two suggestions came up: have a wrapper around the HTTPParser
which we would use as the async resource, or stop reusing HTTPParser in
our HTTP server/client code.

This commit implements the latter, since we have a better GC now and
reusing HTTPParser might make sense anymore. This also will avoid one
extra JS->C++ call in some cases, which should improve performance for
these cases.
addaleax pushed a commit to Drieger/node that referenced this issue Apr 21, 2019
Change resource being used, previously HTTParser was being reused.
We are now using IncomingMessage and ClientRequest objects.  The goal
here is to make the async resource unique for each async operatio

Refs: nodejs#24330
Refs: nodejs/diagnostics#248
Refs: nodejs#21313

Co-authored-by: Matheus Marchini <mat@mmarchini.me>
mmarchini pushed a commit to nodejs/node that referenced this issue Apr 22, 2019
Change resource being used, previously HTTParser was being reused.
We are now using IncomingMessage and ClientRequest objects.  The goal
here is to make the async resource unique for each async operatio

Refs: #24330
Refs: nodejs/diagnostics#248
Refs: #21313

Co-authored-by: Matheus Marchini <mat@mmarchini.me>

PR-URL: #25094
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Flarna added a commit to dynatrace-oss-contrib/node that referenced this issue May 6, 2019
As discussed in nodejs/diagnostics#248,
nodejs#21313 and
https://docs.google.com/document/d/1g8OrG5lMIUhRn1zbkutgY83MiTSMx-0NHDs8Bf-nXxM/preview
reusing the resource object is a blocker for landing a resource based
async hooks API and get rid of the promise destroy hook.

This PR ensures that HttpAgent uses the a new resource object in case
the socket handle gets reused.
@Flarna
Copy link
Member

Flarna commented May 10, 2019

Besides the reuse of HTTPParser I found two more places for reuse in node core:

  • HttpAgent reuses it's handle
  • FileHandle::ReadStart() reuses FileHandleReadWrap (happens at least in Http2)

For the first one I created nodejs/node#27581 to avoid the reuse. Depending on the result of this one I can create one more to avoid the reuse of FileHandleReadWrap.

addaleax pushed a commit to nodejs/node that referenced this issue May 19, 2019
As discussed in nodejs/diagnostics#248,
#21313 and
https://docs.google.com/document/d/1g8OrG5lMIUhRn1zbkutgY83MiTSMx-0NHDs8Bf-nXxM/preview
reusing the resource object is a blocker for landing a resource based
async hooks API and get rid of the promise destroy hook.

This PR ensures that HttpAgent uses the a new resource object in case
the socket handle gets reused.

PR-URL: #27581
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit to nodejs/node that referenced this issue May 20, 2019
As discussed in nodejs/diagnostics#248,
#21313 and
https://docs.google.com/document/d/1g8OrG5lMIUhRn1zbkutgY83MiTSMx-0NHDs8Bf-nXxM/preview
reusing the resource object is a blocker for landing a resource based
async hooks API and get rid of the promise destroy hook.

This PR ensures that HttpAgent uses the a new resource object in case
the socket handle gets reused.

PR-URL: #27581
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@github-actions
Copy link

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

@github-actions github-actions bot added the stale label Jul 17, 2020
@mmarchini
Copy link
Contributor

@Flarna is the FileHandleReadWrap still reused? If so I suggest creating a separate issue for it on nodejs/core and closing this one.

@Flarna
Copy link
Member

Flarna commented Jul 27, 2020

Reuse of FileHandleReadWrap should be fixed by nodejs/node#31972

@Flarna Flarna closed this as completed Jul 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

9 participants