perf: ⚡️ remove extra collection when using rayon by IWANABETHATGUY · Pull Request #2016 · rolldown/rolldown

IWANABETHATGUY · 2024-08-18T17:40:47Z

Description

netlify · 2024-08-18T17:41:19Z

✅ Deploy Preview for rolldown-rs canceled.

Name	Link
🔨 Latest commit	`f6852ef`
🔍 Latest deploy log	https://app.netlify.com/sites/rolldown-rs/deploys/66c238c55d40040008d3317a

hyf0 · 2024-08-18T17:44:35Z

crates/rolldown/src/stages/generate_stage/compute_cross_chunk_links.rs


-    let index_sorted_cross_chunk_imports = index_cross_chunk_imports
-      .into_iter()
-      // FIXME: Extra traversing. This is a workaround due to `par_bridge` doesn't ensure order https://github.com/rayon-rs/rayon/issues/551#issuecomment-882069261


Should keep this. What this PR does is still to be considered as a workaround.

why this is a workaround?

According to rayon-rs/rayon#669 (comment), the ordering now is preserved without another traverse

https://github.com/rolldown/rolldown/pull/2016/files#r1721029588

It's not about rayon here. IMO, this should be solved by the oxc_index to have a native support of rayon.

Then that should be TODO: wait oxc_index support IntoParIterator rather than Fixme:, since the bug is fixed, no overhead needed.

hyf0 · 2024-08-18T17:53:27Z

crates/rolldown/src/utils/chunk/finalize_chunks.rs

+    .raw
    .into_par_iter()
+    .enumerate()
    .map(|(asset_idx, mut hasher)| {


asset_idx become usize instead of AssetIdx, this is unexpected. I'm not sure why oxc_index allows using usize as the valid index type. But the reason we use IndexVec is to have a domain-specific Vec to prevent misusing wrong index. Otherwise we could just use std Vec.

@overlookmotel Sorry for bothering. Could you take have a rough look if this is expected/reasonable behavior of oxc_index?

@hyf0 Sorry I'm so late to come back on this. It got lost in my Github notifications.

I've made a PR to prevent IndexVecs being indexed into with a usize (oxc-project/oxc#5733). As you say, that's a bad idea! That oddity was inherited from index_vec crate.

Opened an issue for the par iter problem: oxc-project/oxc#5734

github-actions · 2024-08-18T18:00:36Z

Benchmarks Rust

target: main(88e1a41)
pr: perf/remove-extra-collection(f6852ef)

group                                                               pr                                     target
-----                                                               --                                     ------
bundle/bundle@multi-duplicated-top-level-symbol                     1.00     51.3±0.80ms        ? ?/sec    1.01     51.8±0.89ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-minify              1.00     75.2±1.39ms        ? ?/sec    1.00     75.3±1.74ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-minify-sourcemap    1.00     90.8±0.99ms        ? ?/sec    1.00     91.0±1.14ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-sourcemap           1.01     60.1±1.27ms        ? ?/sec    1.00     59.8±1.11ms        ? ?/sec
bundle/bundle@rome-ts                                               1.03    103.4±1.54ms        ? ?/sec    1.00    100.5±0.98ms        ? ?/sec
bundle/bundle@rome-ts-minify                                        1.07   194.5±10.56ms        ? ?/sec    1.00    181.9±4.42ms        ? ?/sec
bundle/bundle@rome-ts-minify-sourcemap                              1.02    226.9±7.64ms        ? ?/sec    1.00    221.5±4.73ms        ? ?/sec
bundle/bundle@rome-ts-sourcemap                                     1.01    117.7±1.22ms        ? ?/sec    1.00    116.0±0.89ms        ? ?/sec
bundle/bundle@threejs                                               1.04     37.2±1.13ms        ? ?/sec    1.00     35.9±0.13ms        ? ?/sec
bundle/bundle@threejs-minify                                        1.01     81.9±2.21ms        ? ?/sec    1.00     81.2±1.24ms        ? ?/sec
bundle/bundle@threejs-minify-sourcemap                              1.06    102.1±3.67ms        ? ?/sec    1.00     96.2±1.81ms        ? ?/sec
bundle/bundle@threejs-sourcemap                                     1.03     46.3±0.52ms        ? ?/sec    1.00     45.1±0.47ms        ? ?/sec
bundle/bundle@threejs10x                                            1.02    392.4±3.65ms        ? ?/sec    1.00    385.0±2.04ms        ? ?/sec
bundle/bundle@threejs10x-minify                                     1.02  1001.6±12.93ms        ? ?/sec    1.00    985.4±7.82ms        ? ?/sec
bundle/bundle@threejs10x-minify-sourcemap                           1.00  1223.4±16.93ms        ? ?/sec    1.00  1223.6±11.53ms        ? ?/sec
bundle/bundle@threejs10x-sourcemap                                  1.01    457.2±5.35ms        ? ?/sec    1.00    452.5±2.32ms        ? ?/sec
remapping/remapping                                                 1.01     32.9±0.22ms        ? ?/sec    1.00     32.7±0.13ms        ? ?/sec
remapping/render-chunk-remapping                                    1.03     83.3±0.26ms        ? ?/sec    1.00     81.3±0.32ms        ? ?/sec
scan/scan@rome-ts                                                   1.01     82.2±0.74ms        ? ?/sec    1.00     81.7±0.63ms        ? ?/sec
scan/scan@threejs                                                   1.00     28.7±1.14ms        ? ?/sec    1.00     28.7±0.87ms        ? ?/sec
scan/scan@threejs10x                                                1.00    286.9±2.60ms        ? ?/sec    1.01    289.9±3.04ms        ? ?/sec

IWANABETHATGUY · 2024-08-18T18:05:51Z

TODO: is added

crates/rolldown/src/utils/chunk/finalize_chunks.rs

perf: ⚡️ remove extra collection when using rayon

f9dd015

github-actions bot added the needs-triage label Aug 18, 2024

hyf0 reviewed Aug 18, 2024

View reviewed changes

IWANABETHATGUY added 2 commits August 19, 2024 01:45

chore: 🤖 tweak

d4b049e

chore: 🤖 update lint

4fe44ec

hyf0 reviewed Aug 18, 2024

View reviewed changes

IWANABETHATGUY added the on-hold label Aug 18, 2024

IWANABETHATGUY added 2 commits August 19, 2024 02:02

fix: 🐛 wasi build

44d8e34

chore: 🤖 add some doc

339e5b3

IWANABETHATGUY marked this pull request as ready for review August 18, 2024 18:05

hyf0 reviewed Aug 18, 2024

View reviewed changes

crates/rolldown/src/utils/chunk/finalize_chunks.rs Outdated Show resolved Hide resolved

Apply suggestions from code review

f6852ef

hyf0 approved these changes Aug 18, 2024

View reviewed changes

IWANABETHATGUY added this pull request to the merge queue Aug 19, 2024

Merged via the queue into main with commit 7442d95 Aug 19, 2024

IWANABETHATGUY deleted the perf/remove-extra-collection branch August 19, 2024 02:39

overlookmotel mentioned this pull request Sep 12, 2024

Add ability to iterate over IndexVec with indexes in parallel oxc-project/oxc#5734

Closed

Conversation

IWANABETHATGUY commented Aug 18, 2024

Description

Uh oh!

netlify bot commented Aug 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for rolldown-rs canceled.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks Rust

Uh oh!

IWANABETHATGUY commented Aug 18, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

netlify bot commented Aug 18, 2024 •

edited

Loading

github-actions bot commented Aug 18, 2024 •

edited

Loading