Repository navigation
node-api: use fast properties in node_api_create_object_with_properties - #66470
colinhacks wants to merge 1 commit into
Conversation
node_api_create_object_with_properties() creates every object with v8::Object::New(), which stores the properties in a dictionary. Reads of those objects are about 7x slower than reads of an object literal, and about 70x slower with a null prototype, because then each object also gets a map of its own. Keep a cache of v8::DictionaryTemplate per env, keyed by a hash of the property names. A list of names passed once is only recorded by its hash. When the same list is passed again, a template is created for it, and from then on its objects are instances of the template, with fast properties and one shared map, like object literals. The first 256 lists that are passed again get a template and keep it for the life of the env; later lists keep using v8::Object::New(). A prototype other than Object.prototype costs a SetPrototype() call per object. Names that a template cannot hold keep using v8::Object::New(): symbols, strings that are not one-byte, array indices, duplicate names, and lists of more than 127 names. Add a read operation to the benchmark, which only measured creation. Fixes: nodejs#66441 Assisted-by: Claude Code Signed-off-by: Colin McDonnell <3084745+colinhacks@users.noreply.github.com>
|
Review requested:
|
nodejs-github-bot
added
c++
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #66470 +/- ##
==========================================
+ Coverage 90.17% 90.43% +0.26%
==========================================
Files 769 791 +22
Lines 261448 275644 +14196
Branches 49674 52861 +3187
==========================================
+ Hits 235759 249292 +13533
- Misses 16736 16757 +21
- Partials 8953 9595 +642
🚀 New features to boost your workflow:
|
KevinEady
left a comment
KevinEady
left a comment
There was a problem hiding this comment.
LGTM. Object creation falls back to the previous v8::Object::New() implementation whenever the DictionaryTemplate route is not valid (eg. passing non-Latin-1 string property names, cache hash collisions, array-index string, ...) preserving existing behavior. I cannot think any instance where the new DictionaryTemplate route would provide an object with different properties or behavior (e.g. different insertion order) than the existing implementation.
There was a problem hiding this comment.
In the event of a 32-bit hash collision between two distinct property lists (where list A is already cached), list B will find list A's entry, fail the StrictEquals check, and permanently fall back to object_new().
Since kMaxShapes = 256, collisions are rare, but we can eliminate collision risk entirely with zero allocation overhead by promoting hash and the map key to uint64_t:
// In ObjectShapeCache: uint64_t hash = static_cast<uint64_t>(count); for (size_t i = 0; i < count; i++) { // 64-bit FNV-1a or 64-bit polynomial mixing hash = hash * 1099511628211ULL ^ static_cast<uint64_t>(names[i]->GetIdentityHash()); }
There was a problem hiding this comment.
In the event of a 32-bit hash collision between two distinct property lists (where list A is already cached), list B will find list A's entry, fail the StrictEquals check, and permanently fall back to object_new().
Since kMaxShapes = 256, collisions are rare, but we can eliminate collision risk entirely with zero allocation overhead by promoting hash and the map key to uint64_t:
// In ObjectShapeCache:
uint64_t hash = static_cast<uint64_t>(count);
for (size_t i = 0; i < count; i++) {
// 64-bit FNV-1a or 64-bit polynomial mixing
hash = hash * 1099511628211ULL ^ static_cast<uint64_t>(names[i]->GetIdentityHash());
}And in src/js_native_api_v8.h:
// Change map key from uint32_t to uint64_t:
std::unordered_map<uint64_t, Shape> shapes_;With 64 bits and a maximum of 256 shapes, the probability of a collision is mathematically negligible (~$1.7 \times 10^{-15}$), ensuring no list permanently misses the fast-path without needing vector bucket chaining.
What do you think?
There was a problem hiding this comment.
The fallback logic here is very clean: any non-one-byte name or Symbol correctly bails out with {} and falls back to v8::Object::New(). Storing the empty template in shapes_ also acts as a negative cache, preventing subsequent calls with that shape from repeatedly attempting template creation.
There was a problem hiding this comment.
The slight creation penalty for NULL prototypes (41.6 ns -> 62.2 ns) caused by the SetPrototypeV2 call is definitely worth the 63x property read speedup (53.08 ns -> 0.84 ns) since objects are read far more often than instantiated.
However, calling obj->GetPrototypeV2() queries the prototype from V8 on every single object creation on the hot path, even for standard objects where prototype_or_null is not null.
Solution:
We can avoid calling GetPrototypeV2() on the hot path for standard objects by checking if prototype_or_null is non-default (such as IsNull()) before querying the object's prototype:
// Only query and update prototype if a custom or null prototype was requested:
if (!prototype_or_null.IsEmpty() &&
(prototype_or_null->IsNull() || obj->GetPrototypeV2() != prototype_or_null)) {
if (obj->SetPrototypeV2(context, prototype_or_null).IsNothing()) {
return {};
}
}This ensures standard objects with default Object.prototype skip the extra V8 prototype lookup entirely.
What do you think?
Description
Objects created with
node_api_create_object_with_properties()are in dictionary mode, because the call usesv8::Object::New(). Their properties are about 7x slower to read than an object literal's, and about 70x slower with aNULLprototype, where each object also gets a map of its own.This change keeps a cache of
v8::DictionaryTemplateper env, keyed by a hash of the property names. Core already creates objects from dictionary templates incares_wrap.ccand other places.v8::Object::New().v8::Object::New(), because a template cannot hold them.Object.prototypecosts oneSetPrototype()per object, as inNewDictionaryInstanceNullProto().Fixes #66441
Benchmark
Measured with
benchmark/compare.js, 30 runs per binary, both built from adcd028 on an Apple M1 Max (macOS 26.6) that was also running other work. The benchmark uses 20 properties and aNULLprototype. Every row below has p < 0.001 (Welch's t-test).The rows with
method='old'(napi_create_objectandnapi_set_property) do not use the changed code. They moved by -1% to +7%.Creation with a
NULLprototype is slower because of theSetPrototype()call. WithObject.prototypeit is unchanged. The reproduction from #66441 measures both, with 10 properties and 100,000 objects, in ns per property (median of 3 runs; an object literal reads at 0.78 ns):Object.prototypeNULLValidation
python3 tools/test.py --mode=release js-native-api node-api benchmark/test-benchmark-napiout/Release/cctest --gtest_filter='*NodeApi*:*Napi*'make lint-cpp,git-clang-format, and ESLint on the changed JS filesAI disclosure: I used a coding agent for the investigation, patch, benchmarks, and PR text. It checked the template and
Object::New()behavior against the V8 source, built Node.js, and ran the tests above. I approved the PR and will handle review comments myself.