perf(sourcemap): pre allocate String buf while encoding #4476

Brooooooklyn · 2024-07-26T08:14:21Z

I'm trying llama3.1:70b following local-ai-copilot to find some potential performance improvement.

The code was not completed by AI; it only identified potential optimizations, which I am testing for effectiveness.

graphite-app · 2024-07-26T08:14:26Z

Your org has enabled the Graphite merge queue for merging into main

Add the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

Brooooooklyn · 2024-07-26T08:14:35Z

perf(sourcemap): pre allocate String buf while encoding #4476 👈
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @Brooooooklyn and the rest of your teammates on Graphite

codspeed-hq · 2024-07-26T08:22:14Z

CodSpeed Performance Report

Merging #4476 will improve performances by 3.43%

_{Comparing 07-26-perf_sourcemap_pre_allocate_string_buf_while_encoding (4d10c6c) with main (42a2519)}

Summary

⚡ 1 improvements
✅ 31 untouched benchmarks

Benchmarks breakdown

	Benchmark	`main`	`07-26-perf_sourcemap_pre_allocate_string_buf_while_encoding`	Change
⚡	`sourcemap[cal.com.tsx]`	56.8 ms	54.9 ms	+3.43%

Dunqing · 2024-07-26T08:23:54Z

lol

overlookmotel · 2024-07-26T09:14:53Z

The benchmark result here is not great, but I do think llama is onto something. There is a lot of memory-copying and allocation going on here, and it's largely unnecessary.

I guess the first question is: Does SourceMap::to_json_string needs to return a String?

Presumably main use case for this method is that you call it and then write the output to file. For that use case, the result type only needs to be iterated over, and not indexed into or sliced, and therefore it could be a type which either:

under the hood is more like a Vec<Cow<str>> or
builds output lazily and implements std::io::Read

Brooooooklyn · 2024-07-26T09:39:51Z

Maybe there are some bugs in Codespeed? I haven’t touched any code related the regression cases, and this pr shows the similar regression: #4477 (comment)

overlookmotel · 2024-07-26T10:08:26Z

CodSpeed is using "base" of commit ccb1835 for comparison, whereas the merge commit for this PR that benchmarks are running on is based on 0c13442. So the apparent regression is likely due to 0c13442, not this PR.

What is less clear is why upgrading to Rust 1.80.0 is having such a negative effect on prepass benchmark.

overlookmotel · 2024-07-26T10:36:57Z

#4479 has updated CodSpeed's records of benchmarks to include the regression post-Rust 1.80.0 upgrade.

Do you want to rebase on latest main, so we can get an accurate picture of perf impact of this PR?

crates/oxc_sourcemap/src/encode.rs

graphite-app · 2024-07-26T13:56:26Z

Merge activity

Jul 26, 9:56 AM EDT: The merge label 'merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
Jul 26, 9:56 AM EDT: overlookmotel added this pull request to the Graphite merge queue.
Jul 26, 10:01 AM EDT: overlookmotel merged this pull request with the Graphite merge queue.

I'm trying llama3.1:70b following [local-ai-copilot](https://developer.ibm.com/tutorials/awb-local-ai-copilot-ibm-granite-code-ollama-continue/) to find some potential performance improvement. The code was not completed by AI; it only identified potential optimizations, which I am testing for effectiveness.

Follow up after #4476. Refactor to remove repeated code.

Reduce memory copies when encoding source map as JSON, extending approach taken in #4476 to also avoid memory copies for source texts. I believe reason this shows no benefit on benchmarks is because our benchmarks only create a source map from a single source file, but it should result in a speed-up when there are multiple sources.

## [0.22.1] - 2024-07-27 ### Features - 2477330 ast: Add `AstKind::TSExportAssignment` (#4501) (Dunqing) - aaee07e ast: Add `AstKind::AssignmentTargetPattern`, `AstKind::ArrayAssignmentTarget` and `AstKind::ObjectAssignmentTarget` (#4456) (Dunqing) - fd363d1 ast: Add AstKind::get_container_scope_id (#4450) (DonIsaac) - e2735ca span: Add `contains_inclusive` method (#4491) (DonIsaac) ### Bug Fixes - 368112c ast: Remove `#[visit(ignore)]` from `ExportDefaultDeclarationKind`'s `TSInterfaceDeclaration` (#4497) (Dunqing) - 36bb680 semantic: `TSExportAssignment` cannot reference type binding (#4502) (Dunqing) - cb2fa49 semantic: `typeof` operator cannot reference type-only import (#4500) (Dunqing) - ef0e953 semantic: Generic passed to typeof not counted as a reference (#4499) (Dunqing) - 40cafb8 semantic: Params in `export default (function() {})` flagged as `SymbolFlags::Export` (#4480) (Dunqing) - 2e01a45 semantic: Non-exported namespace member symbols flagged as exported (#4493) (Don Isaac) - e4ca06a semantic: Incorrect symbol’s scope_id after var hoisting (#4458) (Dunqing) - 77bd5f1 semantic: Use correct span for namespace symbols (#4448) (Don Isaac) - 5db7bed sourcemap: Fix pre-calculation of required segments for building JSON (#4490) (overlookmotel) - 1667491 syntax: Correct `is_reserved_keyword_or_global_object`'s incorrect function calling. (#4484) (Ethan Goh) - 82ba2a0 syntax: Fix unsound use of `NonZeroU32` (#4466) (overlookmotel) - c04b9aa transformer: Add to `SymbolTable::declarations` for all symbols (#4460) (overlookmotel) - ecdee88 transformer/typescript: Incorrect eliminate exports when the referenced symbol is both value and type (#4507) (Dunqing) ### Performance - 963a2d1 mangler: Reduce unnecessary allocation (#4498) (Dunqing) - 868fc87 parser: Optimize conditional advance on ASCII values (#4298) (lucab) - 24beaeb semantic: Give `AstNodeId` a niche (#4469) (overlookmotel) - 348c1ad semantic: Remove `span` field from `Reference` (#4464) (overlookmotel) - 6a9f4db semantic: Reduce storage size for symbol redeclarations (#4463) (overlookmotel) - 705e19f sourcemap: Reduce memory copies encoding JSON (#4489) (overlookmotel) - 4d10c6c sourcemap: Pre allocate String buf while encoding (#4476) (Brooooooklyn) ### Documentation - f5f0ba8 ast: Add doc comments to more AST nodes (#4413) (Don Isaac) - 871b3d6 semantic: Add doc comments for SymbolTester and SemanticTester (#4433) (DonIsaac) ### Refactor - 9c5d2f9 ast/builder: Use `Box::new_in` over `.into_in` (#4428) (overlookmotel) - ccb1835 semantic: Methods take `Span` as param, not `&Span` (#4470) (overlookmotel) - f17254a semantic: Populate `declarations` field in `SymbolTable::create_symbol` (#4461) (overlookmotel) - a49f491 semantic: Re-order `SymbolTable` fields (#4459) (overlookmotel) - 7cd53f3 semantic: Var hoisting (#4379) (Dunqing) - 4f5a7cb semantic: Mark SemanticTester and SymbolTester as must_use (#4430) (DonIsaac) - c958a55 sourcemap: `push_list` method for building JSON (#4486) (overlookmotel) - c99b3eb syntax: Give `ScopeId` a niche (#4468) (overlookmotel) - 96fc94f syntax: Use `NonMaxU32` for IDs (#4467) (overlookmotel) ### Testing - 4b274a8 semantic: Add more test cases for symbol references (#4429) (DonIsaac) Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>

Brooooooklyn force-pushed the 07-26-perf_sourcemap_pre_allocate_string_buf_while_encoding branch 3 times, most recently from 7e39c40 to a5fd8a9 Compare July 26, 2024 08:51

Brooooooklyn force-pushed the 07-26-perf_sourcemap_pre_allocate_string_buf_while_encoding branch 3 times, most recently from afb11eb to 5e9e28f Compare July 26, 2024 09:36

Brooooooklyn marked this pull request as ready for review July 26, 2024 09:37

Brooooooklyn mentioned this pull request Jul 26, 2024

test: sourcemap optimization rolldown/rolldown#1745

Closed

This was referenced Jul 26, 2024

Rust 1.80.0 regresses prepass benchmarks by 13% #4478

Closed

ci: run benchmarks on rust-toolchain.toml changes #4479

Merged

Brooooooklyn force-pushed the 07-26-perf_sourcemap_pre_allocate_string_buf_while_encoding branch from 5e9e28f to c521c84 Compare July 26, 2024 10:49

overlookmotel reviewed Jul 26, 2024

View reviewed changes

crates/oxc_sourcemap/src/encode.rs Outdated Show resolved Hide resolved

overlookmotel mentioned this pull request Jul 26, 2024

CodSpeed reporting wrong numbers oxc-project/backlog#82

Open

Brooooooklyn force-pushed the 07-26-perf_sourcemap_pre_allocate_string_buf_while_encoding branch 5 times, most recently from 9e2956b to 4a53188 Compare July 26, 2024 13:13

Brooooooklyn requested a review from overlookmotel July 26, 2024 13:32

Brooooooklyn force-pushed the 07-26-perf_sourcemap_pre_allocate_string_buf_while_encoding branch from 4a53188 to 30b8cc9 Compare July 26, 2024 13:36

overlookmotel requested changes Jul 26, 2024

View reviewed changes

crates/oxc_sourcemap/src/encode.rs Outdated Show resolved Hide resolved

crates/oxc_sourcemap/src/encode.rs Outdated Show resolved Hide resolved

Brooooooklyn force-pushed the 07-26-perf_sourcemap_pre_allocate_string_buf_while_encoding branch from 30b8cc9 to 0b6d2eb Compare July 26, 2024 13:43

overlookmotel approved these changes Jul 26, 2024

View reviewed changes

overlookmotel added the 0-merge Merge with Graphite Merge Queue label Jul 26, 2024

overlookmotel force-pushed the 07-26-perf_sourcemap_pre_allocate_string_buf_while_encoding branch from 0b6d2eb to 4d10c6c Compare July 26, 2024 13:58

graphite-app bot merged commit 4d10c6c into main Jul 26, 2024
23 checks passed

graphite-app bot deleted the 07-26-perf_sourcemap_pre_allocate_string_buf_while_encoding branch July 26, 2024 14:01

This was referenced Jul 26, 2024

refactor(sourcemap): push_list method for building JSON #4486

Merged

perf(sourcemap): reduce memory copies encoding JSON #4489

Merged

Boshen pushed a commit that referenced this pull request Jul 27, 2024

refactor(sourcemap): push_list method for building JSON (#4486)

c958a55

Follow up after #4476. Refactor to remove repeated code.

oxc-bot mentioned this pull request Jul 27, 2024

Release crates v0.22.1 #4508

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(sourcemap): pre allocate String buf while encoding #4476

perf(sourcemap): pre allocate String buf while encoding #4476

Brooooooklyn commented Jul 26, 2024

graphite-app bot commented Jul 26, 2024

Brooooooklyn commented Jul 26, 2024

codspeed-hq bot commented Jul 26, 2024 •

edited

Loading

Dunqing commented Jul 26, 2024

overlookmotel commented Jul 26, 2024

Brooooooklyn commented Jul 26, 2024 •

edited

Loading

overlookmotel commented Jul 26, 2024 •

edited

Loading

overlookmotel commented Jul 26, 2024

graphite-app bot commented Jul 26, 2024 •

edited

Loading

perf(sourcemap): pre allocate String buf while encoding #4476

perf(sourcemap): pre allocate String buf while encoding #4476

Conversation

Brooooooklyn commented Jul 26, 2024

graphite-app bot commented Jul 26, 2024

Your org has enabled the Graphite merge queue for merging into main

Brooooooklyn commented Jul 26, 2024

codspeed-hq bot commented Jul 26, 2024 • edited Loading

CodSpeed Performance Report

Merging #4476 will improve performances by 3.43%

Summary

Benchmarks breakdown

Dunqing commented Jul 26, 2024

overlookmotel commented Jul 26, 2024

Brooooooklyn commented Jul 26, 2024 • edited Loading

overlookmotel commented Jul 26, 2024 • edited Loading

overlookmotel commented Jul 26, 2024

graphite-app bot commented Jul 26, 2024 • edited Loading

Merge activity

codspeed-hq bot commented Jul 26, 2024 •

edited

Loading

Brooooooklyn commented Jul 26, 2024 •

edited

Loading

overlookmotel commented Jul 26, 2024 •

edited

Loading

graphite-app bot commented Jul 26, 2024 •

edited

Loading