feat(core) deno_core::extension! macro to simplify extension registration #18210

mmastrac · 2023-03-15T19:08:58Z

This implements two new macros for extension registration to reduce a lot of the boilerplate and open the door for some future improvements:

deno_core::ops! registers a block of #[op]s, optionally with type parameters.
deno_core::extension! is used to register an extension, and creates two methods that can be used at runtime/snapshot generation time: init_ops and init_ops_and_esm.

An extension may look something like this:

deno_core::extension!(deno_webstorage,
  deps = [ deno_webidl ],
  ops = deno_ops,
  esm = [ "01_webstorage.js" ],
  config = {
    origin_storage_dir: Option<PathBuf>
  },
  state = |state, origin_storage_dir| {
    // ...
  },
);

bartlomieju · 2023-03-16T01:51:13Z

cli/lsp/tsc.rs

+deno_core::extension!(deno_tsc,
+ ops = deno_ops,
+ config = {
+ performance: Arc<Performance>
+ },
+ state = |state, performance| {
+ state.put(State::new(
+ Arc::new(StateSnapshot::default()),
+ performance.clone(),
+ ));
+ },
+);


Sweet! This is exactly what I had in mind. Not sure if config is the name I would go with, but this is the exact idea

Awesome! We can easily rename config as it's only ever used in the macro. I'm happy to do a bikeshedding phase at the end as the consistent macro layout will make that very easy.

cli/tsc/mod.rs

bench_util/benches/op_baseline.rs

core/modules.rs

ext/node/lib.rs

ext/web/benches/timers_ops.rs

bartlomieju · 2023-03-16T14:48:29Z

core/extensions.rs

+macro_rules! ops {
+ ($name:ident, parameters = [ $( $param:ident : $type:ident ),+ ], ops = [ $( $(#[$m:meta])* $( $op:ident )::+ $( < $op_param:ident > )? ),+ $(,)? ]) => {
+ pub(crate) fn $name < $( $param : $type + 'static ),+ > () -> Vec<$crate::OpDecl> {
+ let mut v = vec![];


Would it be possible to use a static slice here?

It's fine to do a vector in the first pass, just asking for a potential followup cleanup/optimization

I believe that we can use a static slice. I believe we can drastically reduce or eliminate the cloning to populate the state that happens as well.

EDIT: op_xxx::decl() isn't const so this isn't possible (yet)

That would be great to see - even with all the improvements from #18080 there are still visible stack in the flamegraph related to initializing ops. Reducing clones and using static slices should help with that 👍

core/extensions.rs

bartlomieju · 2023-03-16T14:50:56Z

cli/ops/testing.rs

+ config = {
+ sender: TestEventSender,
+ fail_fast_tracker: FailFastTracker,
+ filter: TestFilter,
+ },
+ state = |state, sender, fail_fast_tracker, filter| {


@littledivy suggested to me that maybe we should have a single struct instead of config and then state fn would always receive either 1 (state) or 2 (state and config struct) arguments. Any thought @mmastrac?

That would work as well. I actually had that in an earlier pass, but I was worried that having to prefix everything w/cfg. added a lot of noise. I'll circle back at the end on this one as well.

…generic)

…sions

core/extensions.rs

littledivy · 2023-03-17T04:38:26Z

core/extensions.rs

+/// * state: a state initialization function, with the signature `fn (&mut OpState, ...) -> ()`, where `...` are parameters matching the fields of the config struct
+/// * event_loop_middleware: an event-loop middleware function (see [`ExtensionBuilder::event_loop_middleware`])
+#[macro_export]
+macro_rules! extension {


this is sophisticated. we need to have expansion tests like in deno_ops, see https://github.com/eupn/macrotest

@littledivy Do we have a nightly compiler on the CI machines to run the macro expansion tests?

Oh umm yeah we don't use nightly. Let's just have https://github.com/dtolnay/trybuild tests, wdyt?

I like the idea of using macrotest more than trybuild TBH but I think I'd like to split this off into a new PR and experiment a bit. The trybuild tests don't give much signal beyond the codebase compiling and I think there's some value in both seeing the macro output and failing when things change like your nice ops tests.

core/extensions.rs

bartlomieju · 2023-03-17T12:18:15Z

Looks great @mmastrac. I think Divy's suggestions are valid - once we address them this should be good to merge.

ry

I think we should be wary of introducing too much meta-programming. This patch is a net reduction of ~150 lines. But it introduces a lot of complex in macro programming. Because there's no runtime benefit, it's not obvious to me this is a clear win.

bartlomieju · 2023-03-17T13:26:40Z

I think we should be wary of introducing too much meta-programming. This patch is a net reduction of ~150 lines. But it introduces a lot of complex in macro programming. Because there's no runtime benefit, it's not obvious to me this is a clear win.

There's no runtime benefit in this PR, but there will be in a follow up - we will reduce amount of vector allocations significantly when registering extensions on startup and thus make it cheaper to create the main worker instance - there's not gonna be that much difference but I expect to see around 0.5ms improvement in the startup time.

littledivy · 2023-03-17T13:40:59Z

@bartlomieju The static ops list can done without this macro too.

Moreover, there are places where static array won't work.

deno/core/runtime.rs

Line 765 in e6fe163

fn collect_ops(exts: &mut [Extension]) -> Vec<OpDecl> {

Places where the list is mutated or collected into a new Vec...so I think we should first figure out &'static [OpDecl].

mmastrac · 2023-03-17T16:26:33Z

@bartlomieju The static ops list can done without this macro too.

Moreover, there are places where static array won't work.

deno/core/runtime.rs

Line 765 in e6fe163

fn collect_ops(exts: &mut [Extension]) -> Vec<OpDecl> {

Places where the list is mutated or collected into a new Vec...so I think we should first figure out &'static [OpDecl].

The &'static [OpDecl] problem is pretty hairy to tackle without doing this macro and cleanup first, I think.

Barring const OpDecls being available, we have three options:

Create a large allocation at boot time to hold all the OpDecls, and leak that allocation to create a static lifetime reference
Use #[ctor] to initialize the ops before rust's main starts0
Use OnceCell to initialize a static mut array on first access

I'd lean towards the first one, but regardless of which option we choose, it'll require a massive refactoring across the codebase unless we have all the OpDecls handled in one uniform way as we do with this PR. As all the "fast" extensions in the snapshot now have a common implementation, it would be trivial for us to interrogate the size of the OpDecl list in a const manner, allocate a single chunk of memory for all of them (either at runtime or in the compiler) and fill that chunk of memory in the most efficient way possible.

Without this PR landing in every case, we'd basically be making changes to ~20+ op lists across the whole codebase.

Either way, having a macro that's not perfect and exactly what we'll want six months from now is better than nothing, as iterating and experimenting becomes 20x cheaper.

bartlomieju · 2023-03-17T16:52:54Z

Benchmarked against main branch:

./third_party/prebuilt/mac/hyperfine --warmup 10 './target/release/deno run empty.js' './deno_ext run empty.js'
Benchmark #1: ./target/release/deno run empty.js

  Time (mean ± σ):      15.9 ms ±   0.7 ms    [User: 10.6 ms, System: 5.4 ms]

  Range (min … max):    14.9 ms …  19.3 ms

Benchmark #2: ./deno_ext run empty.js

  Time (mean ± σ):      15.6 ms ±   0.6 ms    [User: 10.5 ms, System: 5.4 ms]

  Range (min … max):    14.5 ms …  17.7 ms

Summary

  './deno_ext run empty.js' ran
    1.02x faster than './target/release/deno run empty.js'

This PR cuts startup time by 0.3ms, so IMO it's worth it to land even with additional complexity. It became obvious with Matt's work that there are some unnecessary allocations happening and it's a good forcing function to address them.

littledivy · 2023-03-17T17:06:12Z

Any ideas as to where that improvement comes from?

littledivy

Barring const OpDecls being available, we have three options:

I'm not sure I understand. Extensions holding their individual &'static mut [OpDecl] could just work too.

Without this PR landing in every case, we'd basically be making changes to ~20+ op lists across the whole codebase.

Good point.

mmastrac · 2023-03-17T17:18:31Z

I'm not sure I understand. Extensions holding their individual &'static mut [OpDecl] could just work too.

op_foo::decl() isn't const, so we can't easily create static slices. For X extensions we'd have to leak X Boxes, run X #[ctor] macros, or have X OnceCell-protected arrays.

core/extensions.rs

bartlomieju · 2023-03-17T17:22:43Z

core/ops_builtin.rs

- ops.extend(crate::ops_builtin_v8::init_builtins_v8());
- ext.ops(ops)


One reallocation gone!

bartlomieju · 2023-03-17T17:23:12Z

ext/net/lib.rs

- unsafely_ignore_certificate_errors: Option<Vec<String>>,
-) -> &mut ExtensionBuilder {
- let mut ops = ops::init::<P>();
- ops.extend(ops_tls::init::<P>());


Second reallocation gone 🚀

bartlomieju

LGTM, fantastic first PR.

Please update the commit message before landing (ie. remove mention of ops_bundle!)

littledivy · 2023-03-17T17:26:49Z

op_foo::decl() isn't const, so we can't easily create static slices. For X extensions we'd have to leak X Boxes, run X #[ctor] macros, or have X OnceCell-protected arrays.

Right. decl() shouldn't be hard to make const though.

deno/core/extensions.rs

Lines 48 to 56 in fe07f6c

 pub struct OpDecl { 

 pub name: &'static str, 

 pub v8_fn_ptr: OpFnRef, 

 pub enabled: bool, 

 pub is_async: bool, 

 pub is_unstable: bool, 

 pub is_v8: bool, 

 pub fast_fn: Option<Box<dyn FastFunction>>, 

 }

It's just the fast_fn that can be converted, it can be made a fixed sized trait.

littledivy

LGTM 2 - Great PR

mmastrac · 2023-03-17T17:45:00Z

It's just the fast_fn that can be converted, it can be made a fixed sized trait.

This would be great to have. I think with that we could probably move most of the extension setup to compile-time which would be pretty sweet. I wasn't able to get a good handle on what that particular change would require as it went into some of the magic in rusty_v8.

…nsion

…8252) Follow-up to #18210: * we are passing the generated `cfg` object into the state function rather than passing individual config fields * reduce cloning dramatically by making the state_fn `FnOnce` * `take` for `ExtensionBuilder` to avoid more unnecessary copies * renamed `config` to `options`

mmastrac added the proposal label Mar 15, 2023

mmastrac force-pushed the ops_extension branch from a2750d9 to 7abb161 Compare March 15, 2023 20:50

bartlomieju reviewed Mar 16, 2023

View reviewed changes

ext/web/benches/timers_ops.rs Outdated Show resolved Hide resolved

bartlomieju reviewed Mar 16, 2023

View reviewed changes

core/extensions.rs Outdated Show resolved Hide resolved

bartlomieju reviewed Mar 16, 2023

View reviewed changes

mmastrac added 13 commits March 16, 2023 09:41

WIP: deno_core::ops!

1e33059

WIP extension

dd7f871

WIP extension: napi

782225d

WIP: deno_websocket extension!

ea04949

WIP: extension! for deno_webidl and core

ad224fb

Move state into the macro (though it could still live externally)

910aaa6

Call the state initialization closure using the config fields

e543288

extension! for deno_bench, remove requirement for Default for now

f82ff1e

extension! for deno_console and deno_url

9b3180a

extension! for deno_cli, deno_test, deno_tsc

0ed5c8c

extension! for deno_broadcast_channel (required making Config object …

c55fda0

…generic)

extension! for cache, crypto, ffi, webgpu

702cfda

extension! deno_fetch, deno_flash, deno_bench and some internal exten…

df0112b

…sions

mmastrac force-pushed the ops_extension branch from 0b8d710 to df0112b Compare March 16, 2023 15:41

mmastrac added 3 commits March 16, 2023 09:44

Move ops out of function

c846913

Refactor extension! macro slightly

81dc32a

extension! for cli/build

d78cfb8

mmastrac force-pushed the ops_extension branch from df0112b to c55fda0 Compare March 16, 2023 16:47

mmastrac added 4 commits March 16, 2023 11:28

Restore old init_ops/init_ops_and_esm names

07044e7

extension! for webgpu and webgpu_surface

2f90862

ops -> ops_fn

5076f04

extension! deno_fs

c72730e

mmastrac force-pushed the ops_extension branch from 5343ff9 to c72730e Compare March 16, 2023 17:51

Slighly cleaner approach for runtime_main extension

9a90368

littledivy reviewed Mar 17, 2023

View reviewed changes

core/extensions.rs Show resolved Hide resolved

ry reviewed Mar 17, 2023

View reviewed changes

mmastrac added 2 commits March 17, 2023 08:58

Construct vector in-place rather than using push

3e03787

Merge branch 'main' into ops_extension

ff419ae

littledivy reviewed Mar 17, 2023

View reviewed changes

bartlomieju reviewed Mar 17, 2023

View reviewed changes

core/extensions.rs Outdated Show resolved Hide resolved

bartlomieju reviewed Mar 17, 2023

View reviewed changes

Construct vec! in-place and avoid push

b009d64

bartlomieju approved these changes Mar 17, 2023

View reviewed changes

littledivy approved these changes Mar 17, 2023

View reviewed changes

mmastrac disabled auto-merge March 17, 2023 17:37

mmastrac enabled auto-merge (squash) March 17, 2023 17:40

mmastrac added 2 commits March 17, 2023 11:56

Merge branch 'main' of https://github.com/denoland/deno into ops_exte…

93a9367

…nsion

Missed #[inline] on two functions

0727418

mmastrac merged commit e55b448 into denoland:main Mar 17, 2023

mmastrac deleted the ops_extension branch March 17, 2023 18:26

mmastrac mentioned this pull request Mar 17, 2023

perf(core) Reduce copying and cloning in extension initialization #18252

Merged

bajtos mentioned this pull request Mar 23, 2023

feat: upgrade Deno to 2023.03.22 filecoin-station/zinnia#130

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core) deno_core::extension! macro to simplify extension registration #18210

feat(core) deno_core::extension! macro to simplify extension registration #18210

mmastrac commented Mar 15, 2023 •

edited

Loading

bartlomieju Mar 16, 2023

mmastrac Mar 16, 2023

bartlomieju Mar 16, 2023

bartlomieju Mar 16, 2023 •

edited

Loading

mmastrac Mar 16, 2023 •

edited

Loading

bartlomieju Mar 16, 2023

bartlomieju Mar 16, 2023

mmastrac Mar 16, 2023

littledivy Mar 17, 2023

mmastrac Mar 17, 2023

littledivy Mar 17, 2023

mmastrac Mar 17, 2023

bartlomieju commented Mar 17, 2023

ry left a comment •

edited

Loading

bartlomieju commented Mar 17, 2023

littledivy commented Mar 17, 2023

mmastrac commented Mar 17, 2023

bartlomieju commented Mar 17, 2023

littledivy commented Mar 17, 2023

littledivy left a comment

mmastrac commented Mar 17, 2023

bartlomieju Mar 17, 2023

bartlomieju Mar 17, 2023

bartlomieju left a comment

littledivy commented Mar 17, 2023

littledivy left a comment

mmastrac commented Mar 17, 2023

		ops.extend(crate::ops_builtin_v8::init_builtins_v8());
		ext.ops(ops)

feat(core) deno_core::extension! macro to simplify extension registration #18210

feat(core) deno_core::extension! macro to simplify extension registration #18210

Conversation

mmastrac commented Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartlomieju Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

mmastrac Mar 16, 2023 • edited Loading

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 17, 2023

ry left a comment • edited Loading

Choose a reason for hiding this comment

bartlomieju commented Mar 17, 2023

littledivy commented Mar 17, 2023

mmastrac commented Mar 17, 2023

bartlomieju commented Mar 17, 2023

littledivy commented Mar 17, 2023

littledivy left a comment

Choose a reason for hiding this comment

mmastrac commented Mar 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartlomieju left a comment

Choose a reason for hiding this comment

littledivy commented Mar 17, 2023

littledivy left a comment

Choose a reason for hiding this comment

mmastrac commented Mar 17, 2023

mmastrac commented Mar 15, 2023 •

edited

Loading

bartlomieju Mar 16, 2023 •

edited

Loading

mmastrac Mar 16, 2023 •

edited

Loading

ry left a comment •

edited

Loading