Skip to content

Conversation

@Dunqing
Copy link
Member

@Dunqing Dunqing commented Apr 2, 2025

This method aims to solve many cases that need to wrapped by Box, See rolldown/rolldown#4003 (comment)

Copy link
Member Author

Dunqing commented Apr 2, 2025


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

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

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

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

@codspeed-hq
Copy link

codspeed-hq bot commented Apr 2, 2025

CodSpeed Instrumentation Performance Report

Merging #10169 will create unknown performance changes

Comparing 04-02-feat_allocator_add_takein_take_in_box_method (c198578) with main (a788a64)

Summary

🆕 36 new benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
🆕 codegen[checker.ts] N/A 22.1 ms N/A
🆕 codegen_sourcemap[checker.ts] N/A 65.1 ms N/A
🆕 formatter[antd.js] N/A 7.6 ms N/A
🆕 formatter[react.development.js] N/A 43.4 µs N/A
🆕 formatter[typescript.js] N/A 7.6 ms N/A
🆕 isolated-declarations[vue-id.ts] N/A 58.2 ms N/A
🆕 lexer[RadixUIAdoptionSection.jsx] N/A 21.4 µs N/A
🆕 lexer[antd.js] N/A 24.8 ms N/A
🆕 lexer[cal.com.tsx] N/A 5.9 ms N/A
🆕 lexer[checker.ts] N/A 14.8 ms N/A
🆕 lexer[pdf.mjs] N/A 3.9 ms N/A
🆕 linter[RadixUIAdoptionSection.jsx] N/A 2.7 ms N/A
🆕 linter[cal.com.tsx] N/A 1.2 s N/A
🆕 linter[checker.ts] N/A 3 s N/A
🆕 mangler[antd.js] N/A 15.9 ms N/A
🆕 mangler[react.development.js] N/A 292.9 µs N/A
🆕 mangler[typescript.js] N/A 39.4 ms N/A
🆕 minifier[antd.js] N/A 166.3 ms N/A
🆕 minifier[react.development.js] N/A 1.9 ms N/A
🆕 minifier[typescript.js] N/A 293.2 ms N/A
... ... ... ... ...

ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.

@Dunqing Dunqing marked this pull request as ready for review April 2, 2025 06:30
@Boshen Boshen requested a review from overlookmotel April 2, 2025 08:25
@graphite-app graphite-app bot added the 0-merge Merge with Graphite Merge Queue label Apr 4, 2025
@graphite-app
Copy link
Contributor

graphite-app bot commented Apr 4, 2025

Merge activity

This method aims to solve many cases that need to wrapped by `Box`, See rolldown/rolldown#4003 (comment)
@graphite-app graphite-app bot force-pushed the 04-02-feat_allocator_add_takein_take_in_box_method branch from 9d82508 to c198578 Compare April 4, 2025 07:55
@graphite-app graphite-app bot merged commit c198578 into main Apr 4, 2025
30 checks passed
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Apr 4, 2025
@graphite-app graphite-app bot deleted the 04-02-feat_allocator_add_takein_take_in_box_method branch April 4, 2025 08:10
@overlookmotel
Copy link
Member

@Dunqing I'm not sure if this is ideal way to implement it. I think what this is doing is:

  1. Construct dummy T on stack.
  2. Pull original T onto stack.
  3. Overwrite original T with dummy T in arena.
  4. Allocate original T back into arena again -> Box<T>.

Whereas this would be preferable:

  1. Construct dummy T on stack.
  2. Allocate dummy into arena, creating a Box<T>.
  3. Replace original Box<T> with dummy Box<T>.

The original T doesn't need to be pulled out of arena and than allocated back in again. Also, compiler may be smart enough to see it can construct the dummy directly in arena (combining steps 1 + 2), in which case it avoids large types on the stack entirely.

I should have thought about how to handle Box when I designed TakeIn. My bad.

Perhaps we should implement take_in_box on Box instead?

impl<'a, T: Dummy> for Box<'a, T> {
    fn take_in_box(&mut self, allocator: &'a Allocator) -> Self {
        let dummy = Box::new_in(T::dummy(allocator), allocator);
        mem::replace(self, dummy)
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-enhancement Category - New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants