refactor(core): new optimized op-layer using serde_v8 #9843

AaronO · 2021-03-20T13:48:22Z

This is the first PR in a series refactoring the op-layer to be both simpler and faster, it implements ideas shared in #9540

Keep in mind that given the breadth of changes, the intermediate code does not reflect the final intended state.

The bulk of the changes in this PR are scoped to core/

Key ideas

Use serde_v8 for a maximally efficient bijection between rust <> js values
Simplify and unify op-layer
- Remove concept of custom op-encodings (no longer needed)
- Handle op-metadata (errors, promise IDs) explicitly in the op-layer vs per op-encoding (aka: out-of-payload)
- Remove shared queue & custom "asyncHandlers", all async values are returned in batches via js_recv_cb
- The op-layer should be thought of as simple function calls with little indirection or translation besides the conceptually straightforward serde_v8 bijections
Preserve concepts of json/bin/min as semantic groups of their inputs/outputs instead of their op-encoding strategy, preserving these groups will also facilitate partial transitions over to v8 Fast API for the "min" and "bin" groups

Changes

Follow-up PRs

Change op-sig, removing variadic BufVec
More op-layer optimizations
Revisit plugin ops (related to feat(plugin_api): Align DispatchOpFn signature to that of OpFn and access to OpState #9850)
Tweak all ops to follow best-practices (i.e: avoid using serde_json::Value and json!())
Optimize url_parse
Optimize metrics_op since it increases our baseline op-overhead (accounting for nearly 50% of the baseline iirc, after all the other optimizations have been made)
Optimize op_now by switching it to a json-op or introducing min-ops (first candidates for v8 Fast API)
Add op baseline-overhead benches in core/ (similar to https://gist.github.com/AaronO/41803d5fc131417221a78c911cd3c6b0) (✅ Done: perf(core): op_baseline bench #9924)
Consider renaming bin-ops to io-ops (possibly a better fit for their semantic group)
Update docstrings, docs & core/README to reflect new op-layer
Follow up on promiseTable performance (obj vs map vs ?)

Notes

This temporarily breaks http_bench_bin_ops, it will fail due to the runtime assertion of bin-ops requiring a non-null buffer arg. In practice all of deno's bin-ops follow this and this assertion will be removed by the follow-up PR 1. removing the variadic BufVec
This does not contain all of the op-layer optimizations
It aims to get a first working version with minimal breaking changes elsewhere

core/core.js

Handle missing values (undefined/nullish) by implementing visit_unit for ParseBooleanOrStringVec

Since otherwise casting f64 to u64 will fail, which can happen when unpacking serde_json::Values like in op_close (i.e: Value::as_u64)

Since it only tests the shared queue which no longer exists

Removing variadic zeroCopy, make it an optional arg

core/runtime.rs

ry · 2021-03-28T15:22:57Z

I've given this a cursory review and the structure looks good to me.

core/bindings.rs

core/core.js

core/plugin_api.rs

core/runtime.rs

runtime/metrics.rs

runtime/ops/worker_host.rs

serde_v8/src/de.rs

Since there's no longer a shared queue to overflow

To be re-enabled and revisited prior to 1.9 release

…c assert

bartlomieju

LGTM 🚀

Huge thanks @AaronO! This work will be ground-breaking for the overall performance of the runtime and I'm looking forward to removing even more code in the follow up PRs.

bartlomieju · 2021-03-31T10:46:14Z

core/core.js

@@ -1,411 +1,124 @@
 // Copyright 2018-2021 the Deno authors. All rights reserved. MIT license.


Awesome to see this file being 120 lines

bartlomieju · 2021-03-31T10:47:41Z

core/lib.rs

@@ -56,12 +55,17 @@ pub use crate::modules::RecursiveModuleLoad;
 pub use crate::normalize_path::normalize_path;
 pub use crate::ops::op_close;
 pub use crate::ops::op_resources;
+pub use crate::ops::serialize_op_result;


I don't think this function should be public, what's the use case?

I can make that private, I had left it public since it wasn't 100% clear how we'll approach plugin ops (though it seems like they won't need this and should be able to align plugins with ops without it)

Let's make it non-public for now

bartlomieju · 2021-03-31T10:48:47Z

core/ops.rs

+  }
+
+  pub fn deserialize<T: DeserializeOwned>(self) -> Result<T, AnyError> {
+    serde_v8::from_v8(self.scope.unwrap(), self.value.unwrap())


Is it expected that this function panics for OpPayload::empty()? A bit surpising

In production these should never be None, we only use OpPayload::empty() for the op-table test, so we don't have to spin up a v8 isolate just to test that

So maybe add #[test] attribute to OpPayload::empty

bartlomieju · 2021-03-31T10:52:00Z

core/runtime.rs

+    let async_responses_size = async_responses.len();
+    if async_responses_size == 0 {
      return Ok(());
    }



Side note: this conditional could potentially be removed after this PR - please see FIXME below

Yeah, that FIXME seemed odd to me, since we call JsRuntime.core_js_init() in JsRuntime::new(), so I don't think this can ever happen unless you poll the runtime's event-loop while snapshotting ...

Yes, there was a specific condition to trigger the problem and it was because of shared queue, but since shared queue is gone it shouldn't happen anymore.

core/runtime.rs

bartlomieju · 2021-03-31T12:59:00Z

@AaronO can you please draft a commit message for this PR

`Deno.core.sharedQueue` was killed in denoland#9843

…1503) `Deno.core.sharedQueue` was killed in #9843

- Improves op performance. - Handle op-metadata (errors, promise IDs) explicitly in the op-layer vs per op-encoding (aka: out-of-payload). - Remove shared queue & custom "asyncHandlers", all async values are returned in batches via js_recv_cb. - The op-layer should be thought of as simple function calls with little indirection or translation besides the conceptually straightforward serde_v8 bijections. - Preserve concepts of json/bin/min as semantic groups of their inputs/outputs instead of their op-encoding strategy, preserving these groups will also facilitate partial transitions over to v8 Fast API for the "min" and "bin" groups

AaronO mentioned this pull request Mar 24, 2021

core: introduce serde_v8 #9722

Merged

7 tasks

bartlomieju reviewed Mar 26, 2021

View reviewed changes

core/core.js Outdated Show resolved Hide resolved

AaronO closed this Mar 27, 2021

AaronO force-pushed the ops/a-new-hope branch from af45eff to f46e39c Compare March 27, 2021 00:05

AaronO added 6 commits March 27, 2021 22:49

Mostly working new op layer

6e6d90a

core: handle arg-parsing errors in async json ops

90a6a7b

core: throw TypeError if bin ops don't have a buffer arg

b689e48

Improve robustness of worker permission parsing

dab7b9d

Handle missing values (undefined/nullish) by implementing visit_unit for ParseBooleanOrStringVec

serde_v8: handle ints & floats in deserialize_any()

48f69d2

Since otherwise casting f64 to u64 will fail, which can happen when unpacking serde_json::Values like in op_close (i.e: Value::as_u64)

serde_v8: relax typecheck for deserialize_bool()

3da0316

AaronO reopened this Mar 28, 2021

AaronO added 3 commits March 28, 2021 04:45

core.js: remove shared queue and custom asyncHandlers

bb0e7ff

core: remove core_test.js

40e6cc5

Since it only tests the shared queue which no longer exists

core: removed shared queue (rust code)

09602ce

AaronO changed the title ~~refactor(core): new optimized op-ABI using serde_v8~~ refactor(core): new optimized op-layer using serde_v8 Mar 28, 2021

AaronO changed the base branch from main to v1.8 March 28, 2021 03:14

AaronO changed the base branch from v1.8 to main March 28, 2021 03:15

ry and others added 4 commits March 28, 2021 08:09

fmt

b46ec07

core: update TS types for jsonOpSync & jsonOpAsync

7e4fad0

Removing variadic zeroCopy, make it an optional arg

core: move PromiseId out of OpFn sig and OpResult

27ba629

http_bench_json_ops: follow json-op best practices

fff1996

ry reviewed Mar 28, 2021

View reviewed changes

core/runtime.rs Outdated Show resolved Hide resolved

ry reviewed Mar 28, 2021

View reviewed changes

core/runtime.rs Outdated Show resolved Hide resolved

bartlomieju reviewed Mar 28, 2021

View reviewed changes

AaronO added 5 commits March 28, 2021 23:16

js_unit_tests: update sendAsyncStackTrace

e66d0b7

js_unit_tests: remove now outdated op-encoding tests

bf1a828

js_unit_tests: in metrics assert 0==bytesReceived==bytesSentControl

7a69611

fmt

95315d1

core/runtime: call overflown responses just async responses

aed7c7e

Since there's no longer a shared queue to overflow

AaronO and others added 12 commits March 30, 2021 23:49

test_plugin: ignore integration test

d901ddb

To be re-enabled and revisited prior to 1.9 release

core/ops_bin: re-enable non-empty buffer assert

f9498f3

http_bench_bin_ops: pass around nopBuffer to comply with bin-op BufVe…

b4098bc

…c assert

core/runtime: add comments to explain async_response args encoding

25da8a6

typo

cd7b3e7

Assign TODO to self

4fd87a3

op_baseline: add nopView arg to satisfy bin-op BufVec assert

63febc8

serde_v8: fail deserialization on non-utf8 strings

ab60c84

core: OpPayload.deserialize() fails with TypeError

6de8ca2

lint

e67a706

disable wpt tests

f04660f

fix wpt tests

caa7b82

bartlomieju approved these changes Mar 31, 2021

View reviewed changes

core/runtime: remove unused test func poll_until_ready()

fd10484

core/runtime: rename overflow_response => async_responses

c441d00

ry merged commit fec1b2a into denoland:main Mar 31, 2021

AaronO deleted the ops/a-new-hope branch March 31, 2021 14:57

AaronO mentioned this pull request Apr 18, 2021

Optimize JSON ops: minimize allocs by decoding v8::Value directly #9540

Closed

AaronO added a commit to AaronO/deno that referenced this pull request Jul 23, 2021

cleanup(runtime): remove last references to Deno.core.sharedQueue

c03192f

`Deno.core.sharedQueue` was killed in denoland#9843

AaronO mentioned this pull request Jul 23, 2021

cleanup(runtime): remove last references to Deno.core.sharedQueue #11503

Merged

ry pushed a commit that referenced this pull request Jul 23, 2021

cleanup(runtime): remove last references to Deno.core.sharedQueue (#1…

3e08b6a

…1503) `Deno.core.sharedQueue` was killed in #9843

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(core): new optimized op-layer using serde_v8 #9843

refactor(core): new optimized op-layer using serde_v8 #9843

AaronO commented Mar 20, 2021 •

edited

Loading

ry commented Mar 28, 2021 •

edited

Loading

bartlomieju left a comment

bartlomieju Mar 31, 2021

bartlomieju Mar 31, 2021

AaronO Mar 31, 2021

bartlomieju Mar 31, 2021

bartlomieju Mar 31, 2021

AaronO Mar 31, 2021

bartlomieju Mar 31, 2021

bartlomieju Mar 31, 2021

AaronO Mar 31, 2021

bartlomieju Mar 31, 2021

bartlomieju commented Mar 31, 2021

		@@ -1,411 +1,124 @@
		// Copyright 2018-2021 the Deno authors. All rights reserved. MIT license.

refactor(core): new optimized op-layer using serde_v8 #9843

refactor(core): new optimized op-layer using serde_v8 #9843

Conversation

AaronO commented Mar 20, 2021 • edited Loading

Key ideas

Changes

Follow-up PRs

Notes

ry commented Mar 28, 2021 • edited Loading

bartlomieju left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartlomieju commented Mar 31, 2021

AaronO commented Mar 20, 2021 •

edited

Loading

ry commented Mar 28, 2021 •

edited

Loading