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

refactor(sourcemap): enable_source_map using Arc<str> instead of String #2779

Merged
merged 2 commits into from Mar 23, 2024

Conversation

underfin
Copy link
Collaborator

The source_name using String caused more memory allocate at rolldown, here using Arc<str> to avoid this.

@github-actions github-actions bot added the A-codegen Area - Code Generation label Mar 21, 2024
Copy link

codspeed-hq bot commented Mar 21, 2024

CodSpeed Performance Report

Merging #2779 will improve performances by 6.04%

Comparing underfin:refactor-enable_source_map (c32a27e) with main (64e4de7)

Summary

⚡ 2 improvements
✅ 32 untouched benchmarks

Benchmarks breakdown

Benchmark main underfin:refactor-enable_source_map Change
codegen_sourcemap[react.development.js] 15.1 ms 14.2 ms +6.04%
codegen_sourcemap[typescript.js] 1.5 s 1.4 s +4.37%

@Boshen
Copy link
Member

Boshen commented Mar 21, 2024

Arc feels so weird here, I wonder whether we can change the API and pass in a reference somewhere. Codegen doesn't really need to own the source map name ...

@underfin
Copy link
Collaborator Author

I want to avoid CodegenOptions to CodegenOptions<'a>, as it will be caused Codegen<'a>. What do you think about
change Codegen::new(source_text: &str, options: CodegenOptions) to Codegen::new(source_name &str, source_text: &str, options: CodegenOptions).

@Boshen
Copy link
Member

Boshen commented Mar 22, 2024

Codegen::new(source_name &str, source_text: &str, options: CodegenOptions) seems better, shall we try this one?

Should it be a filename or a file path though, feels like it should be a &Path?

@underfin
Copy link
Collaborator Author

Should it be a filename or a file path though, feels like it should be a &Path?

The &str is enough for it, the rolldown could cast it or other tools.

@github-actions github-actions bot added A-linter Area - Linter A-minifier Area - Minifier A-transformer Area - Transformer / Transpiler labels Mar 23, 2024
@Boshen Boshen merged commit d9b77d8 into oxc-project:main Mar 23, 2024
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area - Code Generation A-linter Area - Linter A-minifier Area - Minifier A-transformer Area - Transformer / Transpiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants