Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf(core): use static specifier in ExtensionFileSource #18271

Merged
merged 2 commits into from
Mar 18, 2023
Merged

perf(core): use static specifier in ExtensionFileSource #18271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d23cd0c
Reland "perf(core): use static specifier in ExtensionFileSource (#182…
bartlomieju Mar 18, 2023
ab15f29
fix specifier
bartlomieju Mar 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bench_util/benches/utf8.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use deno_core::ExtensionFileSourceCode;
fn setup() -> Vec<Extension> {
vec![Extension::builder("bench_setup")
.js(vec![ExtensionFileSource {
specifier: "setup.js".to_string(),
specifier: "ext:bench_setup/setup.js",
code: ExtensionFileSourceCode::IncludedInBinary(
r#"
const hello = "hello world\n";
Expand Down
2 changes: 1 addition & 1 deletion cli/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,7 @@ deno_core::extension!(
],
customizer = |ext: &mut ExtensionBuilder| {
ext.esm(vec![ExtensionFileSource {
specifier: "runtime/js/99_main.js".to_string(),
specifier: "ext:cli/runtime/js/99_main.js",
code: ExtensionFileSourceCode::LoadedFromFsDuringSnapshot(
std::path::PathBuf::from(deno_runtime::js::PATH_FOR_99_MAIN_JS),
),
Expand Down
41 changes: 12 additions & 29 deletions core/extensions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ impl ExtensionFileSourceCode {

#[derive(Clone, Debug)]
pub struct ExtensionFileSource {
pub specifier: String,
pub specifier: &'static str,
pub code: ExtensionFileSourceCode,
}
pub type OpFnRef = v8::FunctionCallback;
Expand Down Expand Up @@ -189,19 +189,19 @@ macro_rules! extension {
#[allow(unused_variables)]
fn with_js(ext: &mut $crate::ExtensionBuilder) {
$( ext.esm(
$crate::include_js_files!( $( dir $dir_esm , )? $( $esm , )* )
$crate::include_js_files!( $name $( dir $dir_esm , )? $( $esm , )* )
); )?
$(
ext.esm(vec![ExtensionFileSource {
specifier: "ext:setup".to_string(),
specifier: "ext:setup",
code: ExtensionFileSourceCode::IncludedInBinary($esm_setup_script),
}]);
)?
$(
ext.esm_entry_point($esm_entry_point);
)?
$( ext.js(
$crate::include_js_files!( $( dir $dir_js , )? $( $js , )* )
$crate::include_js_files!( $name $( dir $dir_js , )? $( $js , )* )
); )?
}

Expand Down Expand Up @@ -451,28 +451,11 @@ pub struct ExtensionBuilder {

impl ExtensionBuilder {
pub fn js(&mut self, js_files: Vec<ExtensionFileSource>) -> &mut Self {
let js_files =
// TODO(bartlomieju): if we're automatically remapping here, then we should
// use a different result struct that `ExtensionFileSource` as it's confusing
// when (and why) the remapping happens.
js_files.into_iter().map(|file_source| ExtensionFileSource {
specifier: format!("ext:{}/{}", self.name, file_source.specifier),
code: file_source.code,
});
self.js.extend(js_files);
self
}

pub fn esm(&mut self, esm_files: Vec<ExtensionFileSource>) -> &mut Self {
let esm_files = esm_files
.into_iter()
// TODO(bartlomieju): if we're automatically remapping here, then we should
// use a different result struct that `ExtensionFileSource` as it's confusing
// when (and why) the remapping happens.
.map(|file_source| ExtensionFileSource {
specifier: format!("ext:{}/{}", self.name, file_source.specifier),
code: file_source.code,
});
self.esm.extend(esm_files);
self
}
Expand Down Expand Up @@ -584,21 +567,21 @@ impl ExtensionBuilder {
#[cfg(not(feature = "include_js_files_for_snapshotting"))]
#[macro_export]
macro_rules! include_js_files {
(dir $dir:literal, $($file:literal,)+) => {
($name:ident dir $dir:literal, $($file:literal,)+) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change broke the existing code, which is expected. Unfortunately, the include_js_files! documentation is out of sync with the actual implementation, which makes it more difficult to fix this problem.

Current docs:

/// /// Example with "dir" option (for "my_extension"):
/// ```ignore
/// include_js_files!(
///   dir "js",
///   "01_hello.js",
///   "02_goodbye.js",
/// )
/// // Produces following specifiers:
/// - "ext:my_extension/js/01_hello.js"
/// - "ext:my_extension/js/02_goodbye.js"
/// ```

Produces compiler errors like this one:

error: couldn't read ext/mine/js: Is a directory (os error 21)
  --> ext/mine/lib.rs:27:14
   |
27 |           .esm(include_js_files!(
   |  ______________^
28 | |             dir "js",
29 | |             "01_hello.js",
30 | |         ))
   | |_________^
   |
   = note: this error originates in the macro `include_str` which comes from the expansion of the macro `include_js_files` (in Nightly builds, run with -Z macro-backtrace for more info)

error: couldn't read ext/mine/01_hello.js: No such file or directory (os error 2)
  --> ext/mine/lib.rs:27:14
   |
27 |           .esm(include_js_files!(
   |  ______________^
28 | |             dir "js",
29 | |             "01_hello.js",
30 | |         ))
   | |_________^
   |
   = note: this error originates in the macro `include_str` which comes from the expansion of the macro `include_js_files` (in Nightly builds, run with -Z macro-backtrace for more info)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for that @bajtos. Could you help me spot here what's the mistake in docs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bartlomieju sorry for posting a comment that's not actionable; I'll do better next time 🙈

The new version of include_js_files! requires the extension name ($name:ident) as the first argument.

I think this is the correct example:

include_js_files!(
   "my-extension",
   dir "js",
   "01_hello.js",
   "02_goodbye.js",
)

In the end, I decided to bite the bullet and upgrade my code to use deno_core::extension! macro. It wasn't that much extra work, kudos for making the extension! macro easy to use outside of the Deno repo too!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, sorry again for the breaking change. We did all these changes to improve startup performance - both for Deno itself and all embedders. The extension! macro moves as much of the work to "build time" instead of "startup time" so hopefully you can see some reduction in your startup time too. Kudos go to @mmastrac for the macro :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes make sense; I understand why you are making them 👍🏻

Speaking about startup performance - is there any guide for embedders? I am particularly interested in learning how to set up our build to create a snapshot of all JS files we are bundling in our runtime to get that faster startup time you mentioned. I can probably figure this out by reading the source code of Deno core & cli, but it would be much nicer if there were a guide. Having said that, both the guide and snapshotting are pretty much "nice to have" for us right now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bajtos there's no official guide - but long story short - you need to define extension! in your build.rs script when creating a snapshot and include all the files you want. Looking at runtime/build.rs and cli/build.rs (the latter creates a snapshot from existing snapshot) is the fastest way to see it in action. If you could open an issue about it we can look into adding some examples to deno_core.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: #18503

vec![
$($crate::ExtensionFileSource {
specifier: concat!($file).to_string(),
specifier: concat!("ext:", stringify!($name), "/", $file),
code: $crate::ExtensionFileSourceCode::IncludedInBinary(
include_str!(concat!($dir, "/", $file)
)),
},)+
]
};

($($file:literal,)+) => {
($name:ident $($file:literal,)+) => {
vec![
$($crate::ExtensionFileSource {
specifier: $file.to_string(),
specifier: concat!("ext:", stringify!($name), "/", $file),
code: $crate::ExtensionFileSourceCode::IncludedInBinary(
include_str!($file)
),
Expand All @@ -610,21 +593,21 @@ macro_rules! include_js_files {
#[cfg(feature = "include_js_files_for_snapshotting")]
#[macro_export]
macro_rules! include_js_files {
(dir $dir:literal, $($file:literal,)+) => {
($name:ident dir $dir:literal, $($file:literal,)+) => {
vec![
$($crate::ExtensionFileSource {
specifier: concat!($file).to_string(),
specifier: concat!("ext:", stringify!($name), "/", $file),
code: $crate::ExtensionFileSourceCode::LoadedFromFsDuringSnapshot(
std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")).join($dir).join($file)
),
},)+
]
};

($($file:literal,)+) => {
($name:ident $($file:literal,)+) => {
vec![
$($crate::ExtensionFileSource {
specifier: $file.to_string(),
specifier: concat!("ext:", stringify!($name), "/", $file),
code: $crate::ExtensionFileSourceCode::LoadedFromFsDuringSnapshot(
std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")).join($file)
),
Expand Down
30 changes: 25 additions & 5 deletions core/modules.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,7 +432,7 @@ impl ModuleLoader for ExtModuleLoader {
if let Some(file_source) = maybe_file_source {
{
let mut used_esm_sources = self.used_esm_sources.borrow_mut();
let used = used_esm_sources.get_mut(&file_source.specifier).unwrap();
let used = used_esm_sources.get_mut(file_source.specifier).unwrap();
*used = true;
}

Expand Down Expand Up @@ -1047,13 +1047,23 @@ impl ModuleMap {
let main = v8::Boolean::new(scope, info.main);
module_info_arr.set_index(scope, 1, main.into());

let name = v8::String::new(scope, &info.name).unwrap();
let name = v8::String::new_from_one_byte(
scope,
info.name.as_bytes(),
v8::NewStringType::Normal,
)
.unwrap();
module_info_arr.set_index(scope, 2, name.into());

let array_len = 2 * info.requests.len() as i32;
let requests_arr = v8::Array::new(scope, array_len);
for (i, request) in info.requests.iter().enumerate() {
let specifier = v8::String::new(scope, &request.specifier).unwrap();
let specifier = v8::String::new_from_one_byte(
scope,
request.specifier.as_bytes(),
v8::NewStringType::Normal,
)
.unwrap();
requests_arr.set_index(scope, 2 * i as u32, specifier.into());

let asserted_module_type =
Expand All @@ -1079,7 +1089,12 @@ impl ModuleMap {
let arr = v8::Array::new(scope, 3);

let (specifier, asserted_module_type) = elem.0;
let specifier = v8::String::new(scope, specifier).unwrap();
let specifier = v8::String::new_from_one_byte(
scope,
specifier.as_bytes(),
v8::NewStringType::Normal,
)
.unwrap();
arr.set_index(scope, 0, specifier.into());

let asserted_module_type =
Expand All @@ -1088,7 +1103,12 @@ impl ModuleMap {

let symbolic_module: v8::Local<v8::Value> = match &elem.1 {
SymbolicModule::Alias(alias) => {
let alias = v8::String::new(scope, alias).unwrap();
let alias = v8::String::new_from_one_byte(
scope,
alias.as_bytes(),
v8::NewStringType::Normal,
)
.unwrap();
alias.into()
}
SymbolicModule::Mod(id) => {
Expand Down
4 changes: 2 additions & 2 deletions core/runtime.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -712,7 +712,7 @@ impl JsRuntime {
futures::executor::block_on(async {
let id = runtime
.load_side_module(
&ModuleSpecifier::parse(&file_source.specifier)?,
&ModuleSpecifier::parse(file_source.specifier)?,
None,
)
.await?;
Expand Down Expand Up @@ -748,7 +748,7 @@ impl JsRuntime {
// TODO(@AaronO): use JsRuntime::execute_static() here to move src off heap
realm.execute_script(
self.v8_isolate(),
&file_source.specifier,
file_source.specifier,
&file_source.code.load()?,
)?;
}
Expand Down
2 changes: 1 addition & 1 deletion ext/url/benches/url_ops.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ fn setup() -> Vec<Extension> {
deno_url::deno_url::init_ops_and_esm(),
Extension::builder("bench_setup")
.esm(vec![ExtensionFileSource {
specifier: "ext:setup".to_string(),
specifier: "ext:bench_setup/setup",
code: ExtensionFileSourceCode::IncludedInBinary(
r#"import { URL } from "ext:deno_url/00_url.js";
globalThis.URL = URL;
Expand Down
2 changes: 1 addition & 1 deletion ext/web/benches/encoding.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ fn setup() -> Vec<Extension> {
),
Extension::builder("bench_setup")
.esm(vec![ExtensionFileSource {
specifier: "ext:setup".to_string(),
specifier: "ext:bench_setup/setup",
code: ExtensionFileSourceCode::IncludedInBinary(
r#"
import { TextDecoder } from "ext:deno_web/08_text_encoding.js";
Expand Down
2 changes: 1 addition & 1 deletion ext/web/benches/timers_ops.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ fn setup() -> Vec<Extension> {
Extension::builder("bench_setup")
.esm(vec![
ExtensionFileSource {
specifier: "ext:setup".to_string(),
specifier: "ext:bench_setup/setup",
code: ExtensionFileSourceCode::IncludedInBinary(r#"
import { setTimeout, handleTimerMacrotask } from "ext:deno_web/02_timers.js";
globalThis.setTimeout = setTimeout;
Expand Down
2 changes: 1 addition & 1 deletion ext/webidl/benches/dict.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ fn setup() -> Vec<Extension> {
deno_webidl::deno_webidl::init_ops_and_esm(),
Extension::builder("deno_webidl_bench")
.esm(vec![ExtensionFileSource {
specifier: "ext:setup".to_string(),
specifier: "ext:deno_webidl_bench/setup.js",
code: ExtensionFileSourceCode::IncludedInBinary(include_str!(
"dict.js"
)),
Expand Down
2 changes: 1 addition & 1 deletion runtime/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ mod startup_snapshot {
deps = [runtime],
customizer = |ext: &mut deno_core::ExtensionBuilder| {
ext.esm(vec![ExtensionFileSource {
specifier: "js/99_main.js".to_string(),
specifier: "ext:runtime_main/js/99_main.js",
code: deno_core::ExtensionFileSourceCode::IncludedInBinary(
include_str!("js/99_main.js"),
),
Expand Down