Expensive isTraceHTTPEnable #81
Description
Problem
I was doing some performance analysis on my lib and I found the following trace:
The method isTraceHTTPEnable is consuming 58ms to check if the trace is enabled when I process 100k requests with total time of ~7s.
To check if tracing is enabled, this method calls a method inside internalBinding('trace_events') to return a boolean, so this slowdown is likely caused by data being passed from the C++ layer to JavaScript.
Reproduction
You can reproduce this CPU Profile and learn how to visualize in this repo.
Solutions
The first solution I thought of was to use v8 Fast APIs, I don't know much about that, but from what I've read in the docs, since the return value is just a boolean, I think it's eligible to be used.
I also thought about caching these values but that won't work since the user can enable and disable tracing whenever he wants, as per the documentation
If you think is possible to use v8 Fast APIs, I can try to work on a PR.
Activity
ronag commentedon May 14, 2023
@nodejs/performance @nodejs/http
ronag commentedon May 14, 2023
I don't know if I'd consider it that expensive. But yea, that could be faster.
mcollina commentedon May 15, 2023
This can be cached/hoisted easily in jsland (by patching the
enablefunction). It would be a good PR.Good finding @ronag!
tniessen commentedon May 15, 2023
FWIW, the function is not really part of
internalBinding('trace_events')but rather part of V8. I am not aware of a way to use V8 Fast API calls with V8 built-ins, but if it is possible, maybe V8 would appreciate a patch upstream.bnoordhuis commentedon May 15, 2023
That's probably not sound in general. For example, an embedder can enable the
node.httptrace category directly through V8's C++ API.W.r.t. V8's fast API: isTraceCategoryEnabled() and trace() are already V8 builtins. There's probably not much room for improvement there.
What should work is expose the uint8_t pointer from
platform->GetTracingController()->GetCategoryGroupEnabled("node.http")as a single-element Uint8Array and changeisTraceHTTPEnabled()to:H4ad commentedon May 15, 2023
I tried code search but I didn't find any use inside built-ins so maybe not a use -case for that.