feat: Pure JS custom host objects

[lrowe]

Avoids substantial slowdown caused by host objects with embedder fields.

• Depends on: feat: Support custom host objects in ValueSerializer rusty_v8#1322
• Depends on: fix(Symbol): deprecate for_global in favour of for_key and for_api rusty_v8#1324
• Deno bug: Implement serializable and transferrable web platform APIs deno#12067

[CLAassistant]

All committers have signed the CLA.

[mmastrac]

For some reason v8::Symbol::for_global is returning a different symbol to the one defined in 01_core.js.

Is it possible that you are getting a SymbolObject back from the key iterator rather than a Symbol?

[lrowe]

Have tracked this down to a rusty_v8 bug and filed denoland/rusty_v8#1323.

[lrowe]

I think this is now ready for review. The relevant tests in deno pass:

$ ./target/debug/deno test cli/tests/unit/message_channel_test.ts
running 2 tests from ./cli/tests/unit/message_channel_test.ts
messagechannel ... ok (35ms)
messagechannel clone port ... ok (20ms)

ok | 2 passed | 0 failed (146ms)

[lrowe]

Running some benchmarks with this branch it looks like the ~30% slowdown I observed in benchmarking my changes to Deno.serve came specifically from calling createHostObject() even without calling op_create_host_object().

I had thought that ObjectSetPrototypeOf(result, Prototype) was the culprit, but after trying the following options I'm at a loss to explain why one is consistently much faster than the other:

function fromInnerRequest(inner, signal, guard) {
  const request = core.createHostObject(RequestPrototype);
  request[webidl.brand] = webidl.brand;
  request[_request] = inner;
  request[_signal] = signal;
  request[_getHeaders] = () => headersFromHeaderList(inner.headerList, guard);
  return request;
}

... is as slow as the old code with:

function fromInnerRequest(inner, signal, guard) {
  const request = ops.op_create_host_object();
  ObjectSetPrototypeOf(request, RequestPrototype);
  request[_request] = inner;
  request[_signal] = signal;
  request[_getHeaders] = () => headersFromHeaderList(inner.headerList, guard);
  return request;
}

... and is 30% slower than both:

function fromInnerRequest(inner, signal, guard) {
  const request = webidl.createBranded(Request);
  request[core.hostObjectBrand] = core.hostObjectBrand;
  request[_request] = inner;
  request[_signal] = signal;
  request[_getHeaders] = () => headersFromHeaderList(inner.headerList, guard);
  return request;
}

.. or:

function fromInnerRequest(inner, signal, guard) {
  const request = ObjectCreate(RequestPrototype);
  request[webidl.brand] = webidl.brand;
  request[core.hostObjectBrand] = core.hostObjectBrand;
  request[_request] = inner;
  request[_signal] = signal;
  request[_getHeaders] = () => headersFromHeaderList(inner.headerList, guard);
  return request;
}

But either way, deno currently has 32 calls to webidl.createBranded(...) vs one call to createHostObject(), so it seems to makes sense to deprecate createHostObject given only one can be used.

[bartlomieju]

Hey @lrowe, sorry for a slow turnaround. I will review all your PRs in the coming days 👍

[lrowe]

Unfortunately it's not possible to run the CI workflow when rusty_v8 is set to a git checkout:

Run cargo test --locked --release --all-features --bins --tests --examples && cargo test --doc
    Updating git repository `[https://github.com/lrowe/rusty_v8`](https://github.com/lrowe/rusty_v8%60)
    Updating git submodule `[https://chromium.googlesource.com/chromium/src/base/trace_event/common.git`](https://chromium.googlesource.com/chromium/src/base/trace_event/common.git%60)
    Updating git submodule `[https://github.com/denoland/chromium_build.git`](https://github.com/denoland/chromium_build.git%60)
    Updating git submodule `[https://github.com/denoland/chromium_buildtools.git`](https://github.com/denoland/chromium_buildtools.git%60)
    Updating git submodule `[https://chromium.googlesource.com/external/github.com/llvm/llvm-project/libcxx.git`](https://chromium.googlesource.com/external/github.com/llvm/llvm-project/libcxx.git%60)
error: failed to get `v8` as a dependency of package `deno_core v0.218.0 (D:\a\deno_core\deno_core\core)`

Caused by:
  failed to load source for dependency `v8`

Caused by:
  Unable to update https://github.com/lrowe/rusty_v8?rev=2545a6159f7ef174538c84c5a0c0e0949222835c

Caused by:
  failed to update submodule `buildtools`

Caused by:
  failed to update submodule `third_party/libc++/trunk`

Caused by:
  path too long: 'C:/Users/runneradmin/.cargo/git/checkouts/rusty_v8-c975a1bffb547bc0/2545a61/buildtools/third_party/libc++/trunk/test/std/experimental/utilities/propagate_const/propagate_const.class/propagate_const.non-const_observers/explicit_operator_element_type_ptr.pass.cpp'; class=Filesystem (30)

[lrowe]

@bartlomieju it would be great if you could take a look at the associated rusty_v8 change first, denoland/rusty_v8#1322. Not having to recompile v8 makes development much easier.

[bartlomieju]

@lrowe so sorry for a slow turnaround. I'm in the process of doing a rusty_v8 release which will include you PR. I'll ping you here again once it's all done.

[bartlomieju]

@lrowe rusty_v8 is now upgraded on the main branch that includes your change.

[lrowe]

Question for reviewers: What's the best way to deprecate createHostObject?

Per the comment above, it is much slower than assigning the brand directly. The only place it is currently used is in the deno MessagePort code (PR changing that: denoland/deno#21358.)

Should we simply remove createHostObject entirely and ensure the deno PR is merged when deno is updated or should we add a @deprecated JSDoc tag and remove later (I don't see any in deno_core itself)?

[bartlomieju]

Question for reviewers: What's the best way to deprecate createHostObject?

Per the comment above, it is much slower than assigning the brand directly. The only place it is currently used is in the deno MessagePort code (PR changing that: denoland/deno#21358.)

Should we simply remove createHostObject entirely and ensure the deno PR is merged when deno is updated or should we add a @deprecated JSDoc tag and remove later (I don't see any in deno_core itself)?

It's fine to remove it outright, no need to deprecate this one since it's mostly "internal" API

[bartlomieju]

One suggestion, but otherwise LGTM 👍

[lrowe]

UPDATE: added new MessageChannel benchmark, switched to v8::String::new where unused createHostObject has been removed.

Benchmark from denoland/deno#20730 with addition of structuredClone many objects and new MessageChannel to show the performance tradeoff, code that uses host objects is faster at the expense of structuredClone doing more work.

Deno.bench("structuredClone object", () => {
  structuredClone({ foo: "bar" });
});

Deno.bench("structuredClone transferables", () => {
  const buf = new Uint8Array([97]);
  structuredClone(buf, {
    transfer: [buf.buffer],
  });
});

Deno.bench("structuredClone many objects", (b) => {
  const array = Array.from({ length: 1000 }, (_, index) => ({ index }));
  b.start();
  structuredClone(array);
  b.end();
});

Deno.bench("new MessageChannel", () => {
  new MessageChannel();
});

This PR with v8::String::new:

% ./deno-38be48d08 bench bench_structuredClone.js
cpu: Apple M1
runtime: deno 1.38.3 (aarch64-apple-darwin)

file:///Users/lrowe/devel/deno/bench_structuredClone.js
benchmark                          time (avg)        iter/s             (min … max)       p75       p99      p995
----------------------------------------------------------------------------------- -----------------------------
structuredClone object            992.33 ns/iter   1,007,726.1   (967.23 ns … 1.06 µs)      1 µs   1.06 µs   1.06 µs
structuredClone transferables       2.08 µs/iter     479,810.5     (2.04 µs … 2.18 µs)    2.1 µs   2.18 µs   2.18 µs
structuredClone many objects      337.79 µs/iter       2,960.4 (322.29 µs … 497.33 µs) 338.75 µs 460.33 µs 467.21 µs
new MessageChannel                957.36 ns/iter   1,044,540.7    (900.15 ns … 1.4 µs) 986.72 ns    1.4 µs    1.4 µs

This PR with alternative v8::String::new_external_onebyte_static:

% ./deno-8682cf494 bench bench_structuredClone.js
cpu: Apple M1
runtime: deno 1.38.3 (aarch64-apple-darwin)

file:///Users/lrowe/devel/deno/bench_structuredClone.js
benchmark                          time (avg)        iter/s             (min … max)       p75       p99      p995
----------------------------------------------------------------------------------- -----------------------------
structuredClone object              1.04 µs/iter     963,577.4        (1 µs … 1.24 µs)   1.04 µs   1.24 µs   1.24 µs
structuredClone transferables       2.13 µs/iter     468,523.2     (2.09 µs … 2.24 µs)   2.14 µs   2.24 µs   2.24 µs
structuredClone many objects      390.39 µs/iter       2,561.6   (366.58 µs … 2.06 ms) 386.67 µs 504.92 µs 673.96 µs
new MessageChannel                961.41 ns/iter   1,040,141.2    (900.4 ns … 1.83 µs) 942.45 ns   1.83 µs   1.83 µs

Current main at base commit:

% ./deno-d65a29794 bench bench_structuredClone.js
cpu: Apple M1
runtime: deno 1.38.3 (aarch64-apple-darwin)

file:///Users/lrowe/devel/deno/bench_structuredClone.js
benchmark                          time (avg)        iter/s             (min … max)       p75       p99      p995
----------------------------------------------------------------------------------- -----------------------------
structuredClone object            868.58 ns/iter   1,151,298.6   (850.1 ns … 884.7 ns) 874.57 ns  884.7 ns  884.7 ns
structuredClone transferables       2.14 µs/iter     467,373.8      (2.11 µs … 2.2 µs)   2.15 µs    2.2 µs    2.2 µs
structuredClone many objects      230.92 µs/iter       4,330.5  (219.83 µs … 577.5 µs) 230.33 µs 301.46 µs 305.33 µs
new MessageChannel                  6.42 µs/iter     155,668.4    (4.75 µs … 10.63 µs)   6.94 µs  10.63 µs  10.63 µs

UPDATE: This PR with looking up symbol once:

% ./deno-c745aad86 bench bench_structuredClone.js
cpu: Apple M1
runtime: deno 1.38.3 (aarch64-apple-darwin)

file:///Users/lrowe/devel/deno/bench_structuredClone.js
benchmark                          time (avg)        iter/s             (min … max)       p75       p99      p995
----------------------------------------------------------------------------------- -----------------------------
structuredClone object              1.01 µs/iter     988,047.8   (986.45 ns … 1.09 µs)   1.01 µs   1.09 µs   1.09 µs
structuredClone transferables       2.19 µs/iter     456,835.8     (2.17 µs … 2.26 µs)    2.2 µs   2.26 µs   2.26 µs
structuredClone many objects      260.39 µs/iter       3,840.4 (251.79 µs … 369.21 µs) 258.08 µs 333.79 µs 340.58 µs
new MessageChannel                965.38 ns/iter   1,035,856.8   (878.46 ns … 2.22 µs) 932.56 ns   2.22 µs   2.22 µs

[lrowe]

With the benchmark above showing 6x faster usage of host objects vs 50% slower structuredClone of many objects I wonder if we can minimize the structuredClone performance penalty when dealing with many objects.

I could try looking up the Deno.core.hostObject symbol once as a v8::Global then use it per is_host_object invocation.

In addition to the code from this PR in is_host_object called per object there is also the code in the rusty_v8 wrapper which creates the scope here: https://github.com/denoland/rusty_v8/pull/1322/files#diff-d6cd4105f704c4d4e4deeb302ad57c0a59152e1abb911f1fe738954de3761295R60-R72

Does that scope object need to be created per invocation of is_host_object or could it be created once and reused?

[lrowe]

So stashing the symbol global in the serializer cuts out most of the overhead for many objects. Benchmark added above, it's about ~12% hit which seems acceptable given the much larger speed up for working with host objects.

[lrowe]

I think this is ready to merge now. I've tweaked it so that the global symbol is only looked up in op_serializer and not op_deserializer.

[bartlomieju]

LGTM, looking forward to improvements in Deno!

[lrowe]

Thanks @bartlomieju! Note that denoland/deno#21358 will need to be merged when deno's deno_core dependency is updated since createHostObject has been removed.

[bartlomieju]

Yup, I'll cut another release this week