Skip to content

Conversation

@overlookmotel
Copy link
Member

@overlookmotel overlookmotel commented May 16, 2025

Follow-on after #10884.

Pure refactor.

To make it explicit when we're converting u32 to/from usize:

  • Make RawVec's len field private.
  • Make all access to len and cap fields go via getter/setter methods.
  • Name those methods *_u32 and *_usize so it's clear when a conversion is happening.

I also renamed cap_* methods to capacity_* for consistency with Vec::capacity.

I haven't used these methods in RawVec because there's some subtlety around when you want the "raw" cap value, and when you want the value capacity method returns, which is different when T is a ZST. Hopefully we can remove support for ZSTs, and remove this ambiguity.

Copy link
Member Author

overlookmotel commented May 16, 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 May 16, 2025

CodSpeed Instrumentation Performance Report

Merging #11081 will improve performances by 14.32%

Comparing 05-16-refactor_allocator_vec_access_len_and_cap_fields_via_getters_setters (8ec0c74) with main (df4cc8d)

Summary

⚡ 2 improvements
✅ 36 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
mangler[RadixUIAdoptionSection.jsx] 14.4 µs 13.7 µs +4.74%
mangler[cal.com.tsx] 3.5 ms 3 ms +14.32%

@overlookmotel overlookmotel marked this pull request as ready for review May 16, 2025 15:07
@overlookmotel overlookmotel requested a review from Dunqing May 17, 2025 10:15
@graphite-app graphite-app bot force-pushed the 05-16-refactor_allocator_vec_re-order_methods branch from 4aa658d to 565bde0 Compare May 17, 2025 10:25
@graphite-app graphite-app bot force-pushed the 05-16-refactor_allocator_vec_access_len_and_cap_fields_via_getters_setters branch from 43a935e to 0b7da6b Compare May 17, 2025 10:26
@overlookmotel overlookmotel force-pushed the 05-16-refactor_allocator_vec_re-order_methods branch from 565bde0 to d12d30f Compare May 17, 2025 12:05
@overlookmotel overlookmotel force-pushed the 05-16-refactor_allocator_vec_access_len_and_cap_fields_via_getters_setters branch from 0b7da6b to 2798717 Compare May 17, 2025 12:05
@graphite-app graphite-app bot changed the base branch from 05-16-refactor_allocator_vec_re-order_methods to graphite-base/11081 May 17, 2025 14:53
@graphite-app graphite-app bot force-pushed the graphite-base/11081 branch from d12d30f to aa76a16 Compare May 17, 2025 15:00
@graphite-app graphite-app bot force-pushed the 05-16-refactor_allocator_vec_access_len_and_cap_fields_via_getters_setters branch from 2798717 to 29d7d91 Compare May 17, 2025 15:00
@graphite-app graphite-app bot changed the base branch from graphite-base/11081 to main May 17, 2025 15:01
@graphite-app graphite-app bot force-pushed the 05-16-refactor_allocator_vec_access_len_and_cap_fields_via_getters_setters branch from 29d7d91 to 99ad11f Compare May 17, 2025 15:01
@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label May 20, 2025
Copy link
Member

Boshen commented May 20, 2025

Merge activity

  • May 19, 9:42 PM EDT: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • May 19, 9:42 PM EDT: Graphite disabled "merge when ready" on this PR due to: a merge conflict with the target branch; resolve the conflict and try again..
  • May 19, 9:42 PM EDT: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • May 19, 10:03 PM EDT: Boshen added this pull request to the Graphite merge queue.
  • May 20, 2:04 AM UTC: The Graphite merge queue couldn't merge this PR because it had conflicts with the trunk branch.
  • May 19, 10:06 PM EDT: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • May 20, 2:06 AM UTC: The merge label '0-merge' was removed. This PR will no longer be merged by the Graphite merge queue
  • May 20, 8:30 PM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • May 20, 8:30 PM UTC: overlookmotel added this pull request to the Graphite merge queue.
  • May 20, 8:39 PM UTC: Merged by the Graphite merge queue.

@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label May 20, 2025
@overlookmotel overlookmotel force-pushed the 05-16-refactor_allocator_vec_access_len_and_cap_fields_via_getters_setters branch from 99ad11f to bb936f8 Compare May 20, 2025 20:24
@overlookmotel overlookmotel added the 0-merge Merge with Graphite Merge Queue label May 20, 2025
…tters (#11081)

Follow-on after #10884.

Pure refactor.

To make it explicit when we're converting `u32` to/from `usize`:

* Make `RawVec`'s `len` field private.
* Make all access to `len` and `cap` fields go via getter/setter methods.
* Name those methods `*_u32` and `*_usize` so it's clear when a conversion is happening.

I also renamed `cap_*` methods to `capacity_*` for consistency with `Vec::capacity`.

I haven't used these methods in `RawVec` because there's some subtlety around when you want the "raw" `cap` value, and when you want the value `capacity` method returns, which is different when `T` is a ZST. Hopefully we can remove support for ZSTs, and remove this ambiguity.
@graphite-app graphite-app bot force-pushed the 05-16-refactor_allocator_vec_access_len_and_cap_fields_via_getters_setters branch from bb936f8 to 8ec0c74 Compare May 20, 2025 20:31
@graphite-app graphite-app bot merged commit 8ec0c74 into main May 20, 2025
24 checks passed
@graphite-app graphite-app bot deleted the 05-16-refactor_allocator_vec_access_len_and_cap_fields_via_getters_setters branch May 20, 2025 20:39
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label May 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants