perf: reduce per-call overhead in gin_helper hot paths#51596
Conversation
ckerr
left a comment
There was a problem hiding this comment.
-
The blink converter change is a nice shortcut.
-
Locker is a good change. I know I've done that same change in another piece of code in the past, kinda surprised I'd left this one on the table. Good catch.
-
Dictionary::Get() is >2x bigger but worth it IMO.
Get(std::string_view)is the most common call by far. -
I haven't crunched the numbers but I'll take your word for it about the value in symbolizing dictionary keys? My intuition is that we probably use the same 100ish keys over and over again.
-
One minor concern: it would be easy for a future contributor to miss the code comment in the .cc file and accidentally add a
CallMethodWithArgs()caller w/o anEscapableHandleScope. The compiler would allow it and CI might even be green. Let's another code comment before the declaration atshell/common/gin_helper/event_emitter_caller.h:21.
Several gin_helper code paths run on every native event emit, every IPC message, every option-dictionary parse, and every C++ -> JS callback. This change removes a handful of avoidable per-call costs from those paths. EmitEvent / CallMethodWithArgs: - Internalize the event name and the "emit" method name with gin::StringToSymbol so V8 dedupes them against its string table and property lookups can compare by pointer identity instead of hashing a fresh kNormal string on every emit. - Reuse the emitter object as the node::CallbackScope async resource instead of allocating a throwaway v8::Object per emit. - Drop the redundant inner EscapableHandleScope; EmitEvent / CustomEmit already open one immediately around the call. gin_helper::Dictionary: - Internalize property keys via gin::StringToSymbol (matching what gin::DataObjectBuilder and gin::ObjectTemplateBuilder already do). - Replace the unconditional Has() + Get() pair in Get() with a single GetRealNamedProperty() lookup — one V8 boundary crossing per key instead of two, with the same missing-vs-undefined disambiguation. Native interceptor objects (process.env, etc.) are detected with the zero-cost HasNamedLookupInterceptor() hidden-class flag and routed to the Has()/Get() path that drives the interceptor. gin_helper::Locker: - Store v8::Locker inline in std::optional instead of heap-allocating it per construction; this is on the path of every C++ -> JS callback. blink converters: - char16_t::FromV8: read the UTF-16 code unit directly instead of round-tripping V8's internal UTF-16 through UTF-8 and back. Microbenchmark medians (Linux x86-64 testing build): EmitEvent (1 listener) 2806 ns -> 1816 ns (-35%) EmitEvent (no listeners) 2788 ns -> 1800 ns (-35%) Dictionary::Get (per key, all hit) 82.2 ns -> 26.9 ns (-67%) Dictionary::Get (per key, all miss) 55.6 ns -> 18.5 ns (-67%) Dictionary::Set (per key) 121 ns -> 82.6 ns (-32%) gin_helper::Locker ctor/dtor 94.3 ns -> 9.85 ns (-90%) C++ -> JS callback (RepeatingCb) 463 ns -> 301 ns (-35%)
1882fb0 to
80a52aa
Compare
|
needs backport labels |
|
Release Notes Persisted
|
|
I have automatically backported this PR to "41-x-y", please check out #51613 |
|
I have automatically backported this PR to "42-x-y", please check out #51614 |
|
I have automatically backported this PR to "43-x-y", please check out #51615 |
Description of Change
Removes a handful of avoidable per-call costs from
gin_helperpaths that run on every native event emit, IPC message, option-dictionary parse, and C++ → JS callback.EmitEvent/CallMethodWithArgs— internalize the event name and"emit"(gin::StringToSymbol); reuse the emitter as thenode::CallbackScoperesource instead of allocating a throwawayv8::Object; drop the redundant innerEscapableHandleScope.gin_helper::Dictionary— internalize property keys; replaceHas()+Get()inGet()with a singleGetRealNamedProperty()(one V8 boundary crossing instead of two), guarded by a zero-costHasNamedLookupInterceptor()flag check that routes native-interceptor objects (process.env, etc.) to the original path.gin_helper::Locker— storev8::Lockerinline instd::optionalinstead of heap-allocating per construction.Converter<char16_t>::FromV8— read the UTF-16 code unit directly instead of round-tripping through UTF-8.Benchmarks
Linux x86-64 testing build, microbenchmark medians:
EmitEvent(1 listener)EmitEvent(no listeners)Dictionary::Get(per key, all hit)Dictionary::Get(per key, all miss)Dictionary::Set(per key)gin_helper::Lockerctor/dtorbase::RepeatingClosure)Checklist
npm testpassesRelease Notes
Notes: Improved performance of native event emission, IPC dispatch, and option-dictionary parsing.