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: avoid allocations in graph.resolve #515

Merged
merged 3 commits into from
Aug 24, 2024
Merged

perf: avoid allocations in graph.resolve #515

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
28b5d48
perf: avoid redirect allocations in graph.resolve
dsherret Aug 23, 2024
65c727e
clarify comment
dsherret Aug 23, 2024
e5fcdc5
Make resolve return a reference
dsherret Aug 23, 2024
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
25 changes: 10 additions & 15 deletions src/fast_check/range_finder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -671,15 +671,14 @@ impl<'a> PublicRangeFinder<'a> {
/* prefer types */ true,
) {
// only analyze registry specifiers
if let Some(dep_nv) = self
.url_converter
.registry_package_url_to_nv(&dep_specifier)
if let Some(dep_nv) =
self.url_converter.registry_package_url_to_nv(dep_specifier)
{
self.add_pending_nv(&dep_nv, pkg_nv);

self.add_pending_trace(
&dep_nv,
&dep_specifier,
dep_specifier,
ImportedExports::star(),
);
}
Expand Down Expand Up @@ -731,7 +730,7 @@ impl<'a> PublicRangeFinder<'a> {
/* prefer types */ true,
) {
if let Some(module_info) =
self.root_symbol.module_from_specifier(&dep_specifier)
self.root_symbol.module_from_specifier(dep_specifier)
{
let module_exports = module_info.exports(self.root_symbol);

Expand Down Expand Up @@ -864,13 +863,13 @@ impl<'a> PublicRangeFinder<'a> {
/* prefer types */ true,
) {
if let Some(dep_nv) =
self.url_converter.registry_package_url_to_nv(&specifier)
self.url_converter.registry_package_url_to_nv(specifier)
{
if dep_nv == *pkg_nv {
// just add this specifier
self.add_pending_trace(
&dep_nv,
&specifier,
specifier,
ImportedExports::from_file_dep_name(&file_dep.name),
);
} else {
Expand Down Expand Up @@ -947,13 +946,13 @@ impl<'a> PublicRangeFinder<'a> {
) {
if let Some(dep_nv) = self
.url_converter
.registry_package_url_to_nv(&specifier)
.registry_package_url_to_nv(specifier)
{
if dep_nv == *pkg_nv {
// just add this specifier
self.add_pending_trace(
&dep_nv,
&specifier,
specifier,
if parts.is_empty() {
ImportedExports::star_with_default()
} else {
Expand Down Expand Up @@ -1048,7 +1047,7 @@ impl<'a> PublicRangeFinder<'a> {
/* prefer types */ true,
) {
if let Some(dep_nv) =
self.url_converter.registry_package_url_to_nv(&specifier)
self.url_converter.registry_package_url_to_nv(specifier)
{
if dep_nv == *pkg_nv {
let named_exports = match &file_dep.name {
Expand All @@ -1066,11 +1065,7 @@ impl<'a> PublicRangeFinder<'a> {
}
};
// just add this specifier
self.add_pending_trace(
&dep_nv,
&specifier,
named_exports,
);
self.add_pending_trace(&dep_nv, specifier, named_exports);
} else {
// need to analyze the whole package
self.add_pending_nv(&dep_nv, pkg_nv);
Expand Down
2 changes: 1 addition & 1 deletion src/fast_check/transform.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,7 @@ impl<'a> FastCheckTransformer<'a> {
else {
return;
};
if let Some(relative) = self.specifier.make_relative(&resolved_specifier) {
if let Some(relative) = self.specifier.make_relative(resolved_specifier) {
if !relative.starts_with("../") {
src.value = format!("./{}", relative).into();
} else {
Expand Down
72 changes: 40 additions & 32 deletions src/graph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1407,7 +1407,7 @@ impl<'a> ModuleGraphErrorIterator<'a> {
let resolved_specifier =
self.iterator.graph.resolve(&resolved.specifier);
let module_slot =
self.iterator.graph.module_slots.get(&resolved_specifier);
self.iterator.graph.module_slots.get(resolved_specifier);
if let Some(ModuleSlot::Err(ModuleError::Missing(
specifier,
maybe_range,
Expand Down Expand Up @@ -1734,7 +1734,7 @@ impl ModuleGraph {
let specifier = self.resolve(specifier);
self
.module_slots
.get(&specifier)
.get(specifier)
.map_or(false, |ms| matches!(ms, ModuleSlot::Module(_)))
}

Expand All @@ -1754,7 +1754,7 @@ impl ModuleGraph {
/// return any resolution error as the error in the result.
pub fn get(&self, specifier: &ModuleSpecifier) -> Option<&Module> {
let specifier = self.resolve(specifier);
match self.module_slots.get(&specifier) {
match self.module_slots.get(specifier) {
Some(ModuleSlot::Module(module)) => Some(module),
_ => None,
}
Expand All @@ -1774,23 +1774,31 @@ impl ModuleGraph {

/// Resolve a specifier from the module graph following any possible redirects
/// returning the "final" module.
pub fn resolve(&self, specifier: &ModuleSpecifier) -> ModuleSpecifier {
pub fn resolve<'a>(
&'a self,
specifier: &'a ModuleSpecifier,
) -> &'a ModuleSpecifier {
const MAX_REDIRECTS: usize = 10;
let mut redirected_specifier = specifier;
let max_redirects = 10;
let mut seen = HashSet::with_capacity(max_redirects);
seen.insert(redirected_specifier);
while let Some(specifier) = self.redirects.get(redirected_specifier) {
if !seen.insert(specifier) {
log::warn!("An infinite loop of redirections detected.\n Original specifier: {specifier}");
break;
}
if let Some(specifier) = self.redirects.get(specifier) {
// only allocate if there's a redirect
let mut seen = HashSet::with_capacity(MAX_REDIRECTS);
seen.insert(redirected_specifier);
seen.insert(specifier);
redirected_specifier = specifier;
if seen.len() >= max_redirects {
log::warn!("An excessive number of redirections detected.\n Original specifier: {specifier}");
break;
while let Some(specifier) = self.redirects.get(redirected_specifier) {
if !seen.insert(specifier) {
log::warn!("An infinite loop of redirections detected.\n Original specifier: {specifier}");
break;
}
redirected_specifier = specifier;
if seen.len() >= MAX_REDIRECTS {
log::warn!("An excessive number of redirections detected.\n Original specifier: {specifier}");
break;
}
}
}
redirected_specifier.clone()
redirected_specifier
}

/// Resolve a dependency of a referring module providing the string specifier
Expand All @@ -1802,35 +1810,35 @@ impl ModuleGraph {
/// of a code dependency. If `false` a code dependency will be returned in
/// favor of a type dependency. The value should be set to `true` when
/// resolving specifiers for type checking, or otherwise `false`.
pub fn resolve_dependency(
&self,
pub fn resolve_dependency<'a>(
&'a self,
specifier: &str,
referrer: &ModuleSpecifier,
prefer_types: bool,
) -> Option<ModuleSpecifier> {
) -> Option<&'a ModuleSpecifier> {
let referrer = self.resolve(referrer);
if let Some(ModuleSlot::Module(referring_module)) =
self.module_slots.get(&referrer)
self.module_slots.get(referrer)
{
self.resolve_dependency_from_module(
specifier,
referring_module,
prefer_types,
)
} else if let Some(graph_import) = self.imports.get(&referrer) {
} else if let Some(graph_import) = self.imports.get(referrer) {
let dependency = graph_import.dependencies.get(specifier)?;
self.resolve_dependency_from_dep(dependency, prefer_types)
} else {
None
}
}

pub fn resolve_dependency_from_module(
&self,
pub fn resolve_dependency_from_module<'a>(
&'a self,
specifier: &str,
referring_module: &Module,
referring_module: &'a Module,
prefer_types: bool,
) -> Option<ModuleSpecifier> {
) -> Option<&'a ModuleSpecifier> {
match referring_module {
Module::Js(referring_module) => {
let dependency = referring_module.dependencies.get(specifier)?;
Expand All @@ -1843,11 +1851,11 @@ impl ModuleGraph {
}
}

pub fn resolve_dependency_from_dep(
&self,
dependency: &Dependency,
pub fn resolve_dependency_from_dep<'a>(
&'a self,
dependency: &'a Dependency,
prefer_types: bool,
) -> Option<ModuleSpecifier> {
) -> Option<&'a ModuleSpecifier> {
let (maybe_first, maybe_second) = if prefer_types {
(&dependency.maybe_type, &dependency.maybe_code)
} else {
Expand All @@ -1859,7 +1867,7 @@ impl ModuleGraph {
let resolved_specifier = self.resolve(unresolved_specifier);
// Even if we resolved the specifier, it doesn't mean the module is actually
// there, so check in the module slots
match self.module_slots.get(&resolved_specifier) {
match self.module_slots.get(resolved_specifier) {
Some(ModuleSlot::Module(Module::Js(module))) if prefer_types => {
// check for if this module has a types dependency
if let Some(Resolution::Ok(resolved)) = module
Expand All @@ -1869,7 +1877,7 @@ impl ModuleGraph {
{
let resolved_specifier = self.resolve(&resolved.specifier);
if matches!(
self.module_slots.get(&resolved_specifier),
self.module_slots.get(resolved_specifier),
Some(ModuleSlot::Module(_))
) {
return Some(resolved_specifier);
Expand Down Expand Up @@ -1908,7 +1916,7 @@ impl ModuleGraph {
specifier: &ModuleSpecifier,
) -> Result<Option<&Module>, &ModuleError> {
let specifier = self.resolve(specifier);
match self.module_slots.get(&specifier) {
match self.module_slots.get(specifier) {
Some(ModuleSlot::Module(module)) => Ok(Some(module)),
Some(ModuleSlot::Err(err)) => Err(err),
_ => Ok(None),
Expand Down
4 changes: 4 additions & 0 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -952,6 +952,7 @@ console.log(a);
false
),
Some(ModuleSpecifier::parse("https://example.com/jsx-runtime").unwrap())
.as_ref()
);
assert_eq!(
graph.resolve_dependency(
Expand All @@ -962,6 +963,7 @@ console.log(a);
Some(
ModuleSpecifier::parse("https://example.com/jsx-runtime.d.ts").unwrap()
)
.as_ref()
);
assert_eq!(
graph
Expand Down Expand Up @@ -3950,6 +3952,7 @@ export function a(a: A): B {
false
),
Some(ModuleSpecifier::parse("https://example.com/jsx-runtime").unwrap())
.as_ref()
);
assert_eq!(
graph.resolve_dependency(
Expand All @@ -3960,6 +3963,7 @@ export function a(a: A): B {
Some(
ModuleSpecifier::parse("https://example.com/jsx-runtime.d.ts").unwrap()
)
.as_ref()
);
}

Expand Down
12 changes: 5 additions & 7 deletions src/symbols/cross_module.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ fn go_to_file_export<'a>(
referrer_module.specifier(),
/* prefer types */ true,
)
.and_then(|dep| specifier_to_module(&dep));
.and_then(specifier_to_module);

let Some(dep_module) = maybe_dep_module else {
return vec![DefinitionPathNode::Unresolved(DefinitionUnresolved {
Expand Down Expand Up @@ -355,8 +355,7 @@ fn go_to_file_export<'a>(
dep_module.specifier(),
/* prefer_types */ true,
);
let maybe_module =
maybe_specifier.and_then(|s| specifier_to_module(&s));
let maybe_module = maybe_specifier.and_then(specifier_to_module);
let mut visited = HashSet::new();
if let Some(module) = maybe_module {
// todo(dsherret): this could be optimized to use an iterator
Expand Down Expand Up @@ -452,8 +451,7 @@ pub fn resolve_symbol_dep<'a>(
module.specifier(),
/* prefer types */ true,
);
let maybe_module =
maybe_dep_specifier.as_ref().and_then(specifier_to_module);
let maybe_module = maybe_dep_specifier.and_then(specifier_to_module);
let Some(module) = maybe_module else {
return vec![ResolvedSymbolDepEntry::Path(
DefinitionPathNode::Unresolved(DefinitionUnresolved {
Expand Down Expand Up @@ -665,7 +663,7 @@ fn resolve_qualified_name_internal<'a>(
/* prefer types */ true,
);
let specifier_module =
maybe_dep_specifier.and_then(|s| specifier_to_module(&s));
maybe_dep_specifier.and_then(specifier_to_module);
if let Some(module) = specifier_module {
next.extend(resolve_qualified_export_name_internal(
graph,
Expand Down Expand Up @@ -841,7 +839,7 @@ fn exports_and_re_exports_inner<'a>(
module.specifier(),
/* prefer_types */ true,
);
let maybe_module = maybe_specifier.and_then(|s| specifier_to_module(&s));
let maybe_module = maybe_specifier.and_then(specifier_to_module);
if let Some(module) = maybe_module {
let inner = exports_and_re_exports_inner(
module_graph,
Expand Down