Conversation
✅ Deploy Preview for rolldown-rs canceled.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1082 +/- ##
==========================================
- Coverage 87.25% 87.24% -0.01%
==========================================
Files 116 116
Lines 5797 5795 -2
==========================================
- Hits 5058 5056 -2
Misses 739 739 ☔ View full report in Codecov by Sentry. |
hyf0
reviewed
May 10, 2024
crates/rolldown_binding/src/options/plugin/parallel_js_plugin.rs
Outdated
Show resolved
Hide resolved
Contributor
Benchmarks RustBenchmarks Rust |
Member
Be hesitate to support such behavior. Users should use |
CodSpeed Performance ReportMerging #1082 will not alter performanceComparing Summary
|
hyf0
approved these changes
May 10, 2024
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jun 25, 2025
…til needed (#5058) These two lines clones the whole bundle output. This is because `args.bundle` is `Vec<Output>` where `Output` is a enum that contains a `Box`. https://github.com/rolldown/rolldown/blob/35d8e1569d820347646a014fca28ed32457f912c/crates/rolldown_binding/src/options/plugin/js_plugin.rs#L483 https://github.com/rolldown/rolldown/blob/35d8e1569d820347646a014fca28ed32457f912c/crates/rolldown_binding/src/options/plugin/js_plugin.rs#L510 While this `.clone` is inevitable to allow referencing the `bundle` parameter of `generateBundle` hook after the hook has end, the content of `bundle` parameter does not need to have a separate value, because it is readonly (the modification is done via `ChangedOutputs` instead). This PR changes `Box` to `Arc` so that we can share the underlying value. This PR will have a bigger impact if the output is large. For example, `astro build` with https://github.com/withastro/astro.build used ~8GB of memory previously (if the GC does not run frequently enough). With this PR, it now uses ~3GB. refs #1082
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The pr intends to make the the item of
Outputis mutable.First, it should be mutable at rust side and js side, i changed the
bundleargument toVec<Output>atPlugin#generate_bundle/write_bundle, before it using&Vec<Output>, it will let the hook can mutatebundle, so it need to return an newbundle, if you not changed it, just return the original value. It also removeArcatOutput, usingArcis difficult to mutate it.For rust side data pass to js side, it need to serialize and deserialize, it has some overhead. Here is some way to do it at the binding side, here only discuss mutate the item of
Output, not including delete item and add item.A. serialize the
Outputto js side, and mutate it, then serialize it to rust side. It has some unnecessary overhead if we only mutate one field of dada.B. serialize the
Outputto js side but hold on theOutputmutable reference and export the setter function of fileds, if some fields mutate, it only need to serialize corresponding data to rust side.The pr implement
B, it is better thanA. If you have some good ideas, please let me know.I will support the delete the item of
Outputat next pr. It only need to export adeleteItemAPI and usingProxyto intercept it at js side.For add item to
Output, because the data comes from js side, we need to serialize it to rust side and push toOutput. But it hasn't the situation at now, it will be delay util somebody need the feature.