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.

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: reduce allocations in MediaType::from_specifier #7

Merged

Conversation

dsherret
Copy link
Member

@dsherret dsherret commented Apr 1, 2024

This is used a lot in the node resolution.

test tests::from_specifier     ... bench:         199 ns/iter (+/- 17)
test tests::from_specifier_old ... bench:         342 ns/iter (+/- 11)

@dsherret dsherret requested a review from mmastrac April 1, 2024 15:29
// slower here, so continue to use to_lowercase()
let lowercase_str = ext.to_lowercase();
match lowercase_str.as_str() {
"ts" => map_typescript_like(path, Self::TypeScript, Self::Dts),
Copy link
Member Author

@dsherret dsherret Apr 1, 2024

Choose a reason for hiding this comment

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

Might be neat to have a macro that expands "ts" to "ts" | "TS" | "Ts" | "tS"... seems to reduce 268ns -> 220ns

Edit: or maybe something like this might be best:

case_insensitive_match!(ext {
  "ts" => map_typescript_like(path, Self::TypeScript, Self::Dts),
  "mts" => map_typescript_like(path, Self::Mts, Self::Dmts),
  ...etc...
})

...then that could be a proc macro that generates the most optimal matching strategy.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe @nathanwhit could tackle that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could use https://doc.rust-lang.org/std/string/struct.String.html#method.eq_ignore_ascii_case instead? I think we could just create a macro that checks string length and uses that method for each case, and the compiler would be reasonably good at optimizing it.

Copy link
Member Author

@dsherret dsherret Apr 2, 2024

Choose a reason for hiding this comment

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

I tried that and it was slower than the match in this case for some reason (I think 40ns slower). It was faster for tsbuildinfo somewhere else in this file. Edit: Didn't try matching on string length though

@dsherret dsherret merged commit 5632a5a into denoland:main Apr 2, 2024
1 check passed
@dsherret dsherret deleted the perf_reduce_allocations_from_specifier branch April 2, 2024 20:15
dsherret added a commit to denoland/deno that referenced this pull request Apr 2, 2024
satyarohith pushed a commit to denoland/deno that referenced this pull request Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants