sqlite: remove the null prototype from result rows

[araujogui]

Current behavior

Rows returned by get(), all() and iterate() are created with a null prototype. The V8 API that does this produces dictionary-mode objects, and V8 will not cache a prototype transition on a dictionary map, so every row also gets its own freshly allocated hidden class. Rows from the same statement therefore share no shape: they are slow to build, and every property access on them in user code is megamorphic.

Proposal

Drop the null prototype and build rows with v8::DictionaryTemplate, cached per statement. Rows become ordinary objects, and every row of a statement shares one hidden class.

Pros

• all() is 10–37% faster depending on the query; reading the rows afterwards is far cheaper still.
• Consistent with run(), which already returns an ordinary object, and with better-sqlite3.

Cons

• Semver-major.
• Rows can no longer be indexed by untrusted keys without Object.hasOwn() — row.toString and row.constructor start resolving through Object.prototype.
• User code comparing rows against { __proto__: null, ... } breaks.

Prototype pollution is not a concern either way: rows are built by defining own properties directly, so a __proto__ column is an own property and never reaches Object.prototype.

I have benchmarks and a working implementation if there is interest.

[bakkot]

I would be very concerned about it becoming unsafe to index rows with untrusted keys. Is there any way to get performant objects with a null prototype? Possibly with a new V8 API, if that's what it would take?

[aduh95]

Prototype pollution is not a concern either way: rows are built by defining own properties directly, so a __proto__ column is an own property and never reaches Object.prototype.

I don't really understand this statement, and more specifically how the part after the colon is related to the part before it. Whether prototype pollution is a concern will depend on the use-case – it's not always possible to know if the prototype has been tampered with, many folks are happy to simply assume it's not a thing, but whether you like it or not, inheriting from Object.prototype increases the attack surface which, as long you use code you haven't written/reviewed (e.g. from npm), is relatively trivial to exploit.

[Renegade334]

V8 refusing to use an existing map for a null-prototype object is somewhat surprising.

Does the same apply for an object with a common prototype that is itself null-prototyped? (eg. {...} > SQLResultRow.prototype > null)

[araujogui]

I would be very concerned about it becoming unsafe to index rows with untrusted keys. Is there any way to get performant objects with a null prototype? Possibly with a new V8 API, if that's what it would take?

Yes, I went and had a look and it seems like a small V8 change. Given the points raised here, I'd rather go that way than drop the null prototype.

The reason it's slow today is that v8::Object::New(isolate, Null(isolate), names, values, n) always builds a dictionary-mode object, and V8 won't cache a prototype transition when the source map is a dictionary map, so every row ends up allocating a fresh Map. Rows from the same statement share no shape at all, which is why they're slow to build and slow to read.

v8::DictionaryTemplate is the API that would fix that, since it caches one map per (context, template), but it hardcodes Object.prototype in three places in DictionaryTemplateInfo::NewInstance, including a release-mode CHECK_EQ. The good news is that prototype transitions are cached when the source map is fast, and the object-literal map it roots at is fast, so the transition only has to happen once per shape and the existing cache takes it from there. I tried it locally and it came out to 8 lines:

-  DirectHandle<Map> current_map = isolate->factory()->ObjectLiteralMapFromCache(
-      context, num_properties_set);
+  DirectHandle<Map> current_map =
+      Map::TransitionRootMapToPrototypeForNewObject(
+          isolate,
+          isolate->factory()->ObjectLiteralMapFromCache(context,
+                                                        num_properties_set),
+          isolate->factory()->null_value());
-        CHECK_EQ(context->object_function_prototype(), cached_map->prototype());
+        CHECK(IsNull(cached_map->prototype()));
-      isolate->slow_object_with_object_prototype_map(), num_properties_set,
+      isolate->slow_object_with_null_prototype_map(), num_properties_set,

With that, rows come out with a null prototype, in fast-properties mode, and all sharing one map, at the same per-row cost as the Object.prototype version. So the null prototype is basically free once the shape is cached.

Tbh I'm not very familiar with V8 internals, so I may well be missing something here.