Skip to content

Commit 86f70aa

Browse files
goffrieConvex, Inc.
goffrie
authored and
Convex, Inc.
committed
Use V8 external strings for JS module source instead of copying (#38782)
GitOrigin-RevId: 67b80fa15ddc49fa77486c6d2758f46e37180cd2
1 parent f7bbfa1 commit 86f70aa

File tree

17 files changed

+133
-41
lines changed

17 files changed

+133
-41
lines changed

‎crates/application/src/deploy_config.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -956,7 +956,7 @@ impl TryFrom<ComponentDefinitionConfigJson> for ComponentDefinitionConfig {
956956
#[serde(rename_all = "camelCase")]
957957
pub struct ModuleJson {
958958
pub path: String,
959-
pub source: ModuleSource,
959+
pub source: String,
960960
pub source_map: Option<SourceMap>,
961961
pub environment: Option<String>,
962962
}
@@ -972,7 +972,7 @@ impl From<ModuleConfig> for ModuleJson {
972972
) -> ModuleJson {
973973
ModuleJson {
974974
path: path.into(),
975-
source,
975+
source: source.to_string(),
976976
source_map,
977977
environment: Some(environment.to_string()),
978978
}
@@ -997,7 +997,7 @@ impl TryFrom<ModuleJson> for ModuleConfig {
997997
};
998998
Ok(ModuleConfig {
999999
path: parse_module_path(&path)?,
1000-
source,
1000+
source: ModuleSource::new(&source),
10011001
source_map,
10021002
environment,
10031003
})

‎crates/application/src/tests/analyze.rs‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -74,13 +74,13 @@ async fn test_analyze(rt: ProdRuntime) -> anyhow::Result<()> {
7474
let modules = vec![
7575
ModuleConfig {
7676
path: "a.js".parse()?,
77-
source: ISOLATE_SOURCE.to_owned(),
77+
source: ISOLATE_SOURCE.into(),
7878
source_map: None,
7979
environment: ModuleEnvironment::Isolate,
8080
},
8181
ModuleConfig {
8282
path: "b.js".parse()?,
83-
source: NODE_SOURCE.to_owned(),
83+
source: NODE_SOURCE.into(),
8484
source_map: None,
8585
environment: ModuleEnvironment::Node,
8686
},
@@ -186,13 +186,13 @@ export { hello, internalHello };
186186
let modules = vec![
187187
ModuleConfig {
188188
path: "isolate_source.js".parse()?,
189-
source: SAMPLE_SOURCE.to_string(),
189+
source: SAMPLE_SOURCE.into(),
190190
source_map: Some(ISOLATE_SOURCE_MAP.to_string()),
191191
environment: ModuleEnvironment::Isolate,
192192
},
193193
ModuleConfig {
194194
path: "node_source.js".parse()?,
195-
source: SAMPLE_SOURCE.to_string(),
195+
source: SAMPLE_SOURCE.into(),
196196
source_map: Some(NODE_SOURCE_MAP.to_string()),
197197
environment: ModuleEnvironment::Node,
198198
},
@@ -248,19 +248,19 @@ async fn test_analyze_crons(rt: ProdRuntime) -> anyhow::Result<()> {
248248
let modules = vec![
249249
ModuleConfig {
250250
path: "a.js".parse()?,
251-
source: ISOLATE_SOURCE.to_owned(),
251+
source: ISOLATE_SOURCE.into(),
252252
source_map: None,
253253
environment: ModuleEnvironment::Isolate,
254254
},
255255
ModuleConfig {
256256
path: "b.js".parse()?,
257-
source: NODE_SOURCE.to_owned(),
257+
source: NODE_SOURCE.into(),
258258
source_map: None,
259259
environment: ModuleEnvironment::Node,
260260
},
261261
ModuleConfig {
262262
path: "crons.js".parse()?,
263-
source: CRONS_SOURCE_A.to_owned(),
263+
source: CRONS_SOURCE_A.into(),
264264
source_map: None,
265265
environment: ModuleEnvironment::Isolate,
266266
},
@@ -295,7 +295,7 @@ async fn test_analyze_crons(rt: ProdRuntime) -> anyhow::Result<()> {
295295
let application = Application::new_for_tests(&rt).await?;
296296
let modules = vec![ModuleConfig {
297297
path: "crons.js".parse()?,
298-
source: CRONS_SOURCE_A.to_owned(),
298+
source: CRONS_SOURCE_A.into(),
299299
source_map: None,
300300
environment: ModuleEnvironment::Isolate,
301301
}];
@@ -321,7 +321,7 @@ async fn test_analyze_crons(rt: ProdRuntime) -> anyhow::Result<()> {
321321
let application = Application::new_for_tests(&rt).await?;
322322
let modules = vec![ModuleConfig {
323323
path: "crons.js".parse()?,
324-
source: CRONS_SOURCE_B.to_owned(),
324+
source: CRONS_SOURCE_B.into(),
325325
source_map: None,
326326
environment: ModuleEnvironment::Isolate,
327327
}];

‎crates/application/src/tests/auth_config.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ export default {
4343
system_env_var_overrides,
4444
Some(ModuleConfig {
4545
path: "auth.config.js".parse().unwrap(),
46-
source: source.to_owned(),
46+
source: source.into(),
4747
source_map: None,
4848
environment: ModuleEnvironment::Isolate,
4949
}),
@@ -94,7 +94,7 @@ export default {
9494
system_env_var_overrides,
9595
Some(ModuleConfig {
9696
path: "auth.config.js".parse().unwrap(),
97-
source: source.to_owned(),
97+
source: source.into(),
9898
source_map: None,
9999
environment: ModuleEnvironment::Isolate,
100100
}),

‎crates/application/src/tests/schema.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ export default new SchemaDefinition()
5252
let schema = application
5353
.evaluate_schema(ModuleConfig {
5454
path: "schema".parse()?,
55-
source: source.to_owned(),
55+
source: source.into(),
5656
source_map: None,
5757
environment: ModuleEnvironment::Isolate,
5858
})
@@ -109,7 +109,7 @@ export default new SchemaDefinition()
109109
let err = application
110110
.evaluate_schema(ModuleConfig {
111111
path: "schema".parse()?,
112-
source: source.to_owned(),
112+
source: source.into(),
113113
source_map: None,
114114
environment: ModuleEnvironment::Isolate,
115115
})
@@ -155,7 +155,7 @@ export default new SchemaDefinition()
155155
let err = application
156156
.evaluate_schema(ModuleConfig {
157157
path: "schema".parse()?,
158-
source: source.to_owned(),
158+
source: source.into(),
159159
source_map: None,
160160
environment: ModuleEnvironment::Isolate,
161161
})
@@ -201,7 +201,7 @@ export default new SchemaDefinition()
201201
let err = application
202202
.evaluate_schema(ModuleConfig {
203203
path: "schema".parse()?,
204-
source: source.to_owned(),
204+
source: source.into(),
205205
source_map: None,
206206
environment: ModuleEnvironment::Isolate,
207207
})

‎crates/application/src/tests/source_package.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ async fn test_source_package(rt: ProdRuntime) -> anyhow::Result<()> {
3535
let path: CanonicalizedModulePath = "b.js".parse()?;
3636
let config = ModuleConfig {
3737
path: path.clone().into(),
38-
source: NODE_SOURCE.to_owned(),
38+
source: NODE_SOURCE.into(),
3939
source_map: Some(SOURCE_MAP.to_owned()),
4040
environment: ModuleEnvironment::Node,
4141
};
@@ -54,7 +54,7 @@ async fn test_source_package(rt: ProdRuntime) -> anyhow::Result<()> {
5454
download_package(application.modules_storage().clone(), storage_key, sha256).await?;
5555

5656
assert_eq!(result.len(), 1);
57-
assert_eq!(&result[&path].source, NODE_SOURCE);
57+
assert_eq!(&*result[&path].source, NODE_SOURCE);
5858
assert_eq!(
5959
result[&path].source_map.as_ref().map(|s| &s[..]),
6060
Some(SOURCE_MAP)

‎crates/isolate/src/environment/component_definitions.rs‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,10 @@ use deno_core::{
4444
use errors::ErrorMetadata;
4545
use model::{
4646
config::types::ModuleConfig,
47-
modules::module_versions::FullModuleSource,
47+
modules::module_versions::{
48+
FullModuleSource,
49+
ModuleSource,
50+
},
4851
};
4952
use rand_chacha::ChaCha12Rng;
5053
use serde_json::Value as JsonValue;
@@ -537,13 +540,13 @@ impl<RT: Runtime> IsolateEnvironment<RT> for DefinitionEnvironment {
537540
};
538541

539542
let synthetic_module = FullModuleSource {
540-
source: format!(
543+
source: ModuleSource::new(&format!(
541544
"export default {{ export: () => {{ return {} }}, componentDefinitionPath: \
542545
\"{}\", defaultName: \"{}\"}}",
543546
serde_json::to_string(&serialized_def)?,
544547
String::from(def_path.clone()),
545548
default_name_string
546-
),
549+
)),
547550
source_map: None,
548551
};
549552
return Ok(Some((

‎crates/isolate/src/execution_scope.rs‎

Lines changed: 45 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,14 @@
11
use std::{
22
collections::HashMap,
3+
ffi::c_char,
34
marker::PhantomData,
5+
mem,
46
ops::{
57
Deref,
68
DerefMut,
79
},
10+
ptr,
11+
str,
812
sync::Arc,
913
};
1014

@@ -27,7 +31,10 @@ use deno_core::{
2731
};
2832
use errors::ErrorMetadata;
2933
use model::modules::{
30-
module_versions::FullModuleSource,
34+
module_versions::{
35+
FullModuleSource,
36+
ModuleSource,
37+
},
3138
user_error::{
3239
ModuleNotFoundError,
3340
SystemModuleNotFoundError,
@@ -342,8 +349,7 @@ impl<'a, 'b: 'a, RT: Runtime, E: IsolateEnvironment<RT>> ExecutionScope<'a, 'b,
342349

343350
let name_str = v8::String::new(&mut scope, name.as_str())
344351
.ok_or_else(|| anyhow!("Failed to create name string"))?;
345-
let source_str = v8::String::new(&mut scope, &module_source.source)
346-
.ok_or_else(|| anyhow!("Failed to create source string"))?;
352+
let source_str = make_source_string(&mut scope, &module_source.source)?;
347353

348354
let origin = helpers::module_origin(&mut scope, name_str);
349355
let (mut v8_source, options) = match &code_cache {
@@ -476,7 +482,7 @@ impl<'a, 'b: 'a, RT: Runtime, E: IsolateEnvironment<RT>> ExecutionScope<'a, 'b,
476482
let (source, source_map) = system_udf_file(system_path)
477483
.ok_or_else(|| SystemModuleNotFoundError::new(system_path))?;
478484
let result = FullModuleSource {
479-
source: source.to_string(),
485+
source: source.into(),
480486
source_map: source_map.as_ref().map(|s| s.to_string()),
481487
};
482488
timer.finish();
@@ -671,3 +677,38 @@ impl<'a, 'b: 'a, RT: Runtime, E: IsolateEnvironment<RT>> ExecutionScope<'a, 'b,
671677
Ok(())
672678
}
673679
}
680+
681+
fn make_source_string<'s>(
682+
scope: &mut v8::HandleScope<'s, ()>,
683+
module_source: &ModuleSource,
684+
) -> anyhow::Result<v8::Local<'s, v8::String>> {
685+
if module_source.is_ascii() {
686+
// Common case: we can use an external string and skip copying the
687+
// module to the V8 heap
688+
let owned_source: Arc<str> = module_source.source_arc().clone();
689+
// SAFETY: we know that `module_source` is ASCII and we have bumped the
690+
// refcount, so the string will not be mutated or freed until we call
691+
// the destructor
692+
let ptr = owned_source.as_ptr();
693+
let len = owned_source.len();
694+
mem::forget(owned_source);
695+
unsafe extern "C" fn destroy(ptr: *mut c_char, len: usize) {
696+
drop(Arc::from_raw(ptr::from_raw_parts::<str>(
697+
ptr.cast::<u8>().cast_const(),
698+
len,
699+
)));
700+
}
701+
// N.B.: new_external_onebyte_raw takes a mut pointer but it does not mutate it
702+
unsafe {
703+
v8::String::new_external_onebyte_raw(
704+
scope,
705+
ptr.cast::<c_char>().cast_mut(),
706+
len,
707+
destroy,
708+
)
709+
}
710+
} else {
711+
v8::String::new(scope, module_source)
712+
}
713+
.ok_or_else(|| anyhow!("Failed to create source string"))
714+
}

‎crates/isolate/src/isolate2/runner.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -520,7 +520,7 @@ async fn run_request<RT: Runtime>(
520520
let requests = client
521521
.register_module(
522522
module_specifier,
523-
module_metadata.source.clone(),
523+
module_metadata.source.to_string(),
524524
module_metadata.source_map.clone(),
525525
)
526526
.await?;

‎crates/isolate/src/lib.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
#![feature(assert_matches)]
99
#![feature(impl_trait_in_assoc_type)]
1010
#![feature(round_char_boundary)]
11+
#![feature(ptr_metadata)]
1112

1213
mod array_buffer_allocator;
1314
pub mod bundled_js;

‎crates/isolate/src/test_helpers.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,7 @@ use model::{
9898
types::FileStorageEntry,
9999
FileStorageId,
100100
},
101+
modules::module_versions::ModuleSource,
101102
scheduled_jobs::VirtualSchedulerModel,
102103
source_packages::{
103104
types::SourcePackage,
@@ -204,7 +205,7 @@ fn load_test_source() -> anyhow::Result<Vec<ModuleConfig>> {
204205
for module in output.modules {
205206
let config = ModuleConfig {
206207
path: module.path.parse()?,
207-
source: module.source,
208+
source: ModuleSource::new(&module.source),
208209
source_map: module.source_map,
209210
environment: module.environment,
210211
};

0 commit comments

Comments
 (0)