Skip to content

Commit

Permalink
perf(core): Cache source lookups (denoland#14816)
Browse files Browse the repository at this point in the history 
Keep a cache for source maps and source lines. 

We sort of already had a cache argument for source map lookup 
functions but we just passed an empty map instead of storing it. 

Extended it to cache source line lookups as well and plugged it 
into runtime state.
  • Loading branch information
@nayeemrmn
nayeemrmn authored Jun 20, 2022
1 parent 94d369e commit 79b4280
Show file tree
Hide file tree
Showing 6 changed files with 147 additions and 152 deletions.
5 changes: 1 addition & 4 deletions cli/fmt_errors.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@ use deno_core::error::format_file_name;
use deno_core::error::JsError;
use deno_core::error::JsStackFrame;

const SOURCE_ABBREV_THRESHOLD: usize = 150;

// Keep in sync with `/core/error.js`.
pub fn format_location(frame: &JsStackFrame) -> String {
let _internal = frame
Expand Down Expand Up @@ -115,8 +113,7 @@ fn format_maybe_source_line(
let source_line = source_line.unwrap();
// sometimes source_line gets set with an empty string, which then outputs
// an empty source line when displayed, so need just short circuit here.
// Also short-circuit on error line too long.
if source_line.is_empty() || source_line.len() > SOURCE_ABBREV_THRESHOLD {
if source_line.is_empty() {
return "".to_string();
}
if source_line.contains("Couldn't format source line: ") {
Expand Down
29 changes: 10 additions & 19 deletions cli/proc_state.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -694,11 +694,9 @@ impl SourceMapGetter for ProcState {
_ => return None,
}
if let Some((code, maybe_map)) = self.get_emit(&specifier) {
let code = String::from_utf8(code).unwrap();
source_map_from_code(code).or(maybe_map)
source_map_from_code(&code).or(maybe_map)
} else if let Ok(source) = self.load(specifier, None, false) {
let code = String::from_utf8(source.code.to_vec()).unwrap();
source_map_from_code(code)
source_map_from_code(&source.code)
} else {
None
}
Expand Down Expand Up @@ -756,21 +754,14 @@ pub fn import_map_from_text(
Ok(result.import_map)
}

fn source_map_from_code(code: String) -> Option<Vec<u8>> {
let lines: Vec<&str> = code.split('\n').collect();
if let Some(last_line) = lines.last() {
if last_line
.starts_with("//# sourceMappingURL=data:application/json;base64,")
{
let input = last_line.trim_start_matches(
"//# sourceMappingURL=data:application/json;base64,",
);
let decoded_map = base64::decode(input)
.expect("Unable to decode source map from emitted file.");
Some(decoded_map)
} else {
None
}
fn source_map_from_code(code: &[u8]) -> Option<Vec<u8>> {
static PREFIX: &[u8] = b"//# sourceMappingURL=data:application/json;base64,";
let last_line = code.rsplitn(2, |u| u == &b'\n').next().unwrap();
if last_line.starts_with(PREFIX) {
let input = last_line.split_at(PREFIX.len()).1;
let decoded_map = base64::decode(input)
.expect("Unable to decode source map from emitted file.");
Some(decoded_map)
} else {
None
}
Expand Down
140 changes: 76 additions & 64 deletions core/error.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@

use crate::runtime::JsRuntime;
use crate::source_map::apply_source_map;
use crate::source_map::get_source_line;
use crate::url::Url;
use anyhow::Error;
use std::borrow::Cow;
use std::collections::HashMap;
use std::collections::HashSet;
use std::fmt;
use std::fmt::Debug;
Expand Down Expand Up @@ -192,15 +192,20 @@ impl JsError {
let msg = v8::Exception::create_message(scope, exception);

let mut exception_message = None;
let state_rc = JsRuntime::state(scope);
let state = state_rc.borrow();
if let Some(format_exception_cb) = &state.js_format_exception_cb {
let format_exception_cb = format_exception_cb.open(scope);
let this = v8::undefined(scope).into();
let formatted = format_exception_cb.call(scope, this, &[exception]);
if let Some(formatted) = formatted {
if formatted.is_string() {
exception_message = Some(formatted.to_rust_string_lossy(scope));
// Nest this state borrow. A mutable borrow can occur when accessing `stack`
// in this outer scope, invoking `Error.prepareStackTrace()` which calls
// `op_apply_source_map`.
{
let state_rc = JsRuntime::state(scope);
let state = state_rc.borrow();
if let Some(format_exception_cb) = &state.js_format_exception_cb {
let format_exception_cb = format_exception_cb.open(scope);
let this = v8::undefined(scope).into();
let formatted = format_exception_cb.call(scope, this, &[exception]);
if let Some(formatted) = formatted {
if formatted.is_string() {
exception_message = Some(formatted.to_rust_string_lossy(scope));
}
}
}
}
Expand Down Expand Up @@ -255,66 +260,73 @@ impl JsError {
None => vec![],
};

let state_rc = JsRuntime::state(scope);
let state = state_rc.borrow();

// When the stack frame array is empty, but the source location given by
// (script_resource_name, line_number, start_column + 1) exists, this is
// likely a syntax error. For the sake of formatting we treat it like it
// was given as a single stack frame.
if frames.is_empty() {
let script_resource_name = msg
.get_script_resource_name(scope)
.and_then(|v| v8::Local::<v8::String>::try_from(v).ok())
.map(|v| v.to_rust_string_lossy(scope));
let line_number: Option<i64> =
msg.get_line_number(scope).and_then(|v| v.try_into().ok());
let column_number: Option<i64> = msg.get_start_column().try_into().ok();
if let (Some(f), Some(l), Some(c)) =
(script_resource_name, line_number, column_number)
{
// V8's column numbers are 0-based, we want 1-based.
let c = c + 1;
if let Some(source_map_getter) = &state.source_map_getter {
let (f, l, c, _) = apply_source_map(
f,
l,
c,
&mut HashMap::new(),
source_map_getter.as_ref(),
);
frames =
vec![JsStackFrame::from_location(Some(f), Some(l), Some(c))];
} else {
frames =
vec![JsStackFrame::from_location(Some(f), Some(l), Some(c))];
}
}
}

let mut source_line = None;
let mut source_line_frame_index = None;
if let Some(source_map_getter) = &state.source_map_getter {
for (i, frame) in frames.iter().enumerate() {
if let (Some(file_name), Some(line_number)) =
(&frame.file_name, frame.line_number)
{
let state_rc = JsRuntime::state(scope);
let state = &mut *state_rc.borrow_mut();

// When the stack frame array is empty, but the source location given by
// (script_resource_name, line_number, start_column + 1) exists, this is
// likely a syntax error. For the sake of formatting we treat it like it
// was given as a single stack frame.
if frames.is_empty() {
let script_resource_name = msg
.get_script_resource_name(scope)
.and_then(|v| v8::Local::<v8::String>::try_from(v).ok())
.map(|v| v.to_rust_string_lossy(scope));
let line_number: Option<i64> =
msg.get_line_number(scope).and_then(|v| v.try_into().ok());
let column_number: Option<i64> =
msg.get_start_column().try_into().ok();
if let (Some(f), Some(l), Some(c)) =
(script_resource_name, line_number, column_number)
{
if !file_name.trim_start_matches('[').starts_with("deno:") {
// Source lookup expects a 0-based line number, ours are 1-based.
source_line = source_map_getter
.get_source_line(file_name, (line_number - 1) as usize);
source_line_frame_index = Some(i);
break;
// V8's column numbers are 0-based, we want 1-based.
let c = c + 1;
if let Some(source_map_getter) = &state.source_map_getter {
let (f, l, c) = apply_source_map(
f,
l,
c,
&mut state.source_map_cache,
source_map_getter.as_ref(),
);
frames =
vec![JsStackFrame::from_location(Some(f), Some(l), Some(c))];
} else {
frames =
vec![JsStackFrame::from_location(Some(f), Some(l), Some(c))];
}
}
}
} else if let Some(frame) = frames.first() {
if let Some(file_name) = &frame.file_name {
if !file_name.trim_start_matches('[').starts_with("deno:") {
source_line = msg
.get_source_line(scope)
.map(|v| v.to_rust_string_lossy(scope));
source_line_frame_index = Some(0);

if let Some(source_map_getter) = &state.source_map_getter {
for (i, frame) in frames.iter().enumerate() {
if let (Some(file_name), Some(line_number)) =
(&frame.file_name, frame.line_number)
{
if !file_name.trim_start_matches('[').starts_with("deno:") {
// Source lookup expects a 0-based line number, ours are 1-based.
source_line = get_source_line(
file_name,
line_number,
&mut state.source_map_cache,
source_map_getter.as_ref(),
);
source_line_frame_index = Some(i);
break;
}
}
}
} else if let Some(frame) = frames.first() {
if let Some(file_name) = &frame.file_name {
if !file_name.trim_start_matches('[').starts_with("deno:") {
source_line = msg
.get_source_line(scope)
.map(|v| v.to_rust_string_lossy(scope));
source_line_frame_index = Some(0);
}
}
}
}
Expand Down
6 changes: 3 additions & 3 deletions core/ops_builtin_v8.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -752,14 +752,14 @@ fn op_apply_source_map(
location: Location,
) -> Result<Location, Error> {
let state_rc = JsRuntime::state(scope);
let state = state_rc.borrow();
let state = &mut *state_rc.borrow_mut();
if let Some(source_map_getter) = &state.source_map_getter {
let mut location = location;
let (f, l, c, _) = apply_source_map_(
let (f, l, c) = apply_source_map_(
location.file_name,
location.line_number.into(),
location.column_number.into(),
&mut Default::default(),
&mut state.source_map_cache,
source_map_getter.as_ref(),
);
location.file_name = f;
Expand Down
3 changes: 3 additions & 0 deletions core/runtime.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ use crate::modules::NoopModuleLoader;
use crate::op_void_async;
use crate::op_void_sync;
use crate::ops::*;
use crate::source_map::SourceMapCache;
use crate::source_map::SourceMapGetter;
use crate::Extension;
use crate::OpMiddlewareFn;
Expand Down Expand Up @@ -163,6 +164,7 @@ pub(crate) struct JsRuntimeState {
/// of the event loop.
dyn_module_evaluate_idle_counter: u32,
pub(crate) source_map_getter: Option<Box<dyn SourceMapGetter>>,
pub(crate) source_map_cache: SourceMapCache,
pub(crate) pending_ops: FuturesUnordered<PendingOpFuture>,
pub(crate) unrefed_ops: HashSet<i32>,
pub(crate) have_unpolled_ops: bool,
Expand Down Expand Up @@ -391,6 +393,7 @@ impl JsRuntime {
has_tick_scheduled: false,
js_wasm_streaming_cb: None,
source_map_getter: options.source_map_getter,
source_map_cache: Default::default(),
pending_ops: FuturesUnordered::new(),
unrefed_ops: HashSet::new(),
shared_array_buffer_store: options.shared_array_buffer_store,
Expand Down
Loading

0 comments on commit 79b4280

Please sign in to comment.