| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
I’ll mark this as request changes only because it already has an approval but still needs tests in order to land :) But I’m fine with the approach here
Sorry, something went wrong.
|
Hi @addaleax could you please take another look here? |
Sorry, something went wrong.
Sorry, something went wrong.
There are situations where one wants to invoke a JS callback's ->Call()
from C++ and in particular retain any existing async_context state, but
where it's not obvious that a plain ->Call() would be safe at the point
in question.
Such callsites usually resort to
node::MakeCallback(..., async_context{0, 0}), which unconditionally
pushes the async_context{0, 0} and takes the required provisions for the
->Call() itself such as triggering the tick after its return, if needed.
An example would be the PerformanceObserver invocation from
PerformanceEntry::Notify(): this can get called when coming from JS
through e.g. perf_hooks.performance.mark() and alike, but perhaps also
from nghttp2 (c.f. EmitStatistics() in node_http2.cc).
In the former case, a plain ->Call() would be safe and it would be
desirable to retain the current async_context so that
PerformanceObservers can access it resp. the associated
AsyncLocalStorage. However, in the second case the additional provisions
taken by node::MakeCallback() might potentially be strictly required.
So PerformanceEntry::Notify() bites the bullet and invokes the
PerformanceObservers through node::MakeCallback() unconditionally,
thereby always rendering any possibly preexisting async_context
inaccessible.
Introduce the convenience node::MakeSyncCallback() for such usecases,
which would basically forward to ->Call() if safe and to
node::MakeCallback(..., async_context{0, 0}) otherwise.
Co-Authored-By: ZauberNerd <zaubernerd@zaubernerd.de>
PR-URL: nodejs#36343
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
It's desirable to retain async_contexts active at callsites of
perf_hooks.performance.mark() and alike in the subsequent
PerformanceObserver invocations such that the latter can access e.g.
associated AsyncLocalStorage instances.
In working towards this goal replace the node::MakeCallback(...,
async_context{0, 0}) in PerformanceEntry::doNotify() by the new
node::MakeSyncCallback() introduced specifically for this purpose.
This change will retain the original async_context, if any, in
perf_hook's observersCallback() and thus, for the subsequent doNotify()
on unbuffered PerformanceObservers.
Co-Authored-By: ZauberNerd <zaubernerd@zaubernerd.de>
PR-URL: nodejs#36343
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
This reverts commit 009e418. AFAIU the discussion at [1], PerformanceObserver had been made to inherit from AsyncResource more or less as a band-aid in lack of a better async_context candidate to invoke it in. In order to enable access to AsyncLocalStores from PerformanceObservers invoked synchronously through e.g. measure() or mark(), the current async_context, if any, should be retained. Note that this is a breaking change, but - as has been commented at [1], PerformanceObserver being derived from AsyncResource is a "minor divergence from the spec" anyway, - to my knowledge this is an internal implementation detail which has never been documented and - I can't think of a good reason why existing PerformanceObserver implementations would possibly rely on it. OTOH, it's probably worthwhile to not potentially invoke before() and after() async_hooks for each and every PerformanceObserver notification. [1] nodejs#18789 Co-Authored-By: ZauberNerd <zaubernerd@zaubernerd.de> PR-URL: nodejs#36343 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>
This test proves that the PerformanceObserver callback gets called with the async context of the callsite of performance.mark()/measure() and therefore AsyncLocalStorage can be used inside a PerformanceObserver. PR: nodejs#36343 PR-URL: nodejs#36343 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>
It's desirable to retain async_contexts active at callsites of
perf_hooks.performance.mark() and alike in the subsequent
PerformanceObserver invocations such that the latter can access e.g.
associated AsyncLocalStorage instances.
In working towards this goal replace the node::MakeCallback(...,
async_context{0, 0}) in PerformanceEntry::doNotify() by the new
node::MakeSyncCallback() introduced specifically for this purpose.
This change will retain the original async_context, if any, in
perf_hook's observersCallback() and thus, for the subsequent doNotify()
on unbuffered PerformanceObservers.
Co-Authored-By: ZauberNerd <zaubernerd@zaubernerd.de>
PR-URL: #36343
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
This reverts commit 009e418. AFAIU the discussion at [1], PerformanceObserver had been made to inherit from AsyncResource more or less as a band-aid in lack of a better async_context candidate to invoke it in. In order to enable access to AsyncLocalStores from PerformanceObservers invoked synchronously through e.g. measure() or mark(), the current async_context, if any, should be retained. Note that this is a breaking change, but - as has been commented at [1], PerformanceObserver being derived from AsyncResource is a "minor divergence from the spec" anyway, - to my knowledge this is an internal implementation detail which has never been documented and - I can't think of a good reason why existing PerformanceObserver implementations would possibly rely on it. OTOH, it's probably worthwhile to not potentially invoke before() and after() async_hooks for each and every PerformanceObserver notification. [1] #18789 Co-Authored-By: ZauberNerd <zaubernerd@zaubernerd.de> PR-URL: #36343 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>
This test proves that the PerformanceObserver callback gets called with the async context of the callsite of performance.mark()/measure() and therefore AsyncLocalStorage can be used inside a PerformanceObserver. PR: #36343 PR-URL: #36343 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>
| Back | FazBrowse Home | New Git URL |
Problem description:
Using the perf_hooks module (specifically the User Timing API), to collect timings in an async context (i.e. to collect metrics during the lifetime of an HTTP request), it is not possible to correlate the PerformanceEntrys with a single request (except for using workarounds, such as prefixing all mark names with a unique id).
This is required, when implementing e.g. tracing solutions, where one wants to have timings per requests.
Solution:
With the changes in this PR we are able to retain async contexts when calling the PerformanceObserver, which enables us to, for example, access an AsyncLocalStorage in the observer callback to store the collected metrics per request.
Additional notes:
Questions:
Checklist