Skip to content

Commit a4f1cb4

Browse files
committed
turbo-tasks: address PR #93835 review comments
- Introduce `Tag(NonZeroU8)` newtype in `lazy_tail.rs` to carry the "1..=LAZY_N" invariant at the type level and centralize the bit/mask helpers (`bit`, `mask_below`, `mask_above`, `table_index`). Macro emits `LAZY_TAG_<NAME>: Tag` constants and dispatch fns take `Tag`. `Option<Tag>` is 1 byte via the niche. - Add a Lemire reference to `sum_padded_sizes`'s loop and note the stdlib has no built-in set-bit iterator. - Add unit tests for `sum_padded_sizes`, `offset_of`, the mask- partition invariant, and the `Option<Tag>` niche. - Rename `LazyTail::install_unchecked` -> `insert_unchecked` and `TaskStorage::lazy_install` -> `lazy_insert`. Cross-reference `replace_in_place` from `insert_unchecked`'s safety doc. - Add a `const _` assert pinning `size_of::<Option<TaskStorage>>() == size_of::<TaskStorage>()`. - Inline imports for `TaskStorage` in `backend::mod` and `backend::operation` (drop the fully-qualified spellings). - Fix a stale doc reference (`TaskStorageInner::evictability` -> `TaskStorage::evictability`) and drop a stale `CachedDataItem` mention from the schema module docs. - Rename `TurboMalloc::good_size` -> `get_bucket_size`. All `backend::lazy_tail::tests` (8) and `backend::storage_schema::tests` (16) pass under cargo test and Miri's default Stacked Borrows mode.
1 parent 81ea5a3 commit a4f1cb4

8 files changed

Lines changed: 315 additions & 84 deletions

File tree

turbopack/crates/turbo-tasks-backend/src/backend/lazy_tail.rs

Lines changed: 241 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,11 @@
11
//! Bitmap + byte-counter record for `TaskStorageInner`'s lazy fields.
22
//!
33
//! Each lazy variant is assigned a 1-based tag at macro time (see
4-
//! `LAZY_TAG_*` constants). A presence bitmap (`present`) records which tags
5-
//! have a payload stored; the byte buffer holds payloads packed in tag order.
6-
//! Per-tag size and alignment are looked up via the `LAZY_SIZE`,
7-
//! `LAZY_ALIGN`, and `LAZY_PADDED_SIZE` tables emitted by the schema.
4+
//! `LAZY_TAG_*` constants, all typed as [`Tag`]). A presence bitmap
5+
//! (`present`) records which tags have a payload stored; the byte buffer
6+
//! holds payloads packed in tag order. Per-tag size and alignment are
7+
//! looked up via the `LAZY_SIZE`, `LAZY_ALIGN`, and `LAZY_PADDED_SIZE`
8+
//! tables emitted by the schema.
89
//!
910
//! `LazyTail` is just the bookkeeping (presence bitmap + used/allocated
1011
//! byte counts). The byte buffer itself lives directly after the
@@ -14,6 +15,8 @@
1415
//! does the alloc/dealloc/realloc, then passes a `tail_base: *mut u8`
1516
//! pointer into `LazyTail`'s typed methods so they can read/write payloads.
1617
18+
use std::num::NonZeroU8;
19+
1720
use turbo_tasks::ShrinkToFit;
1821

1922
use crate::backend::storage_schema::LAZY_PADDED_SIZE;
@@ -23,6 +26,74 @@ use crate::backend::storage_schema::LAZY_PADDED_SIZE;
2326
/// `LAZY_MAX_ALIGN` is asserted equal at construction time.
2427
pub(crate) const TAIL_BUFFER_ALIGN: usize = 8;
2528

29+
/// A 1-based lazy-variant tag. Constructed from the schema-emitted
30+
/// `LAZY_TAG_<NAME>` constants. Tag 0 is reserved as the bincode encode
31+
/// sentinel and never appears as a `Tag` value — that invariant is carried
32+
/// at the type level via [`NonZeroU8`], so `Option<Tag>` also gets a
33+
/// 1-byte niche representation.
34+
///
35+
/// The presence bitmap on [`LazyTail`] uses bit `tag - 1` for each variant,
36+
/// so this newtype centralizes the bit/mask math instead of repeating
37+
/// `1u32 << (tag - 1)` everywhere.
38+
#[repr(transparent)]
39+
#[derive(Copy, Clone, PartialEq, Eq, Debug)]
40+
pub(crate) struct Tag(NonZeroU8);
41+
42+
impl Tag {
43+
/// Construct from a raw 1-based tag. Compiles to a no-op when `raw` is
44+
/// a non-zero literal.
45+
///
46+
/// # Panics
47+
/// Panics if `raw == 0` (in both debug and release) and in debug if
48+
/// `raw > 32`. Callers should use the schema-emitted `LAZY_TAG_<NAME>`
49+
/// constants rather than constructing tags directly.
50+
#[inline]
51+
pub(crate) const fn new(raw: u8) -> Self {
52+
debug_assert!(raw <= 32, "Tag exceeds bitmap capacity (max 32)");
53+
match NonZeroU8::new(raw) {
54+
Some(n) => Self(n),
55+
None => panic!("Tag must be 1-based; raw=0 is the sentinel"),
56+
}
57+
}
58+
59+
/// The raw 1-based tag value, used for wire serialization.
60+
#[inline]
61+
pub(crate) const fn raw(self) -> u8 {
62+
self.0.get()
63+
}
64+
65+
/// Index into the per-tag tables (`LAZY_SIZE`, `LAZY_ALIGN`,
66+
/// `LAZY_PADDED_SIZE`). Identical to `raw()` for 1-based tags — slot 0
67+
/// in those tables is reserved.
68+
#[inline]
69+
pub(crate) const fn table_index(self) -> usize {
70+
self.0.get() as usize
71+
}
72+
73+
/// The single-bit mask `1u32 << (raw - 1)`. Used to check presence and
74+
/// to set / clear this variant's bit in the bitmap.
75+
#[inline]
76+
pub(crate) const fn bit(self) -> u32 {
77+
1u32 << (self.0.get() - 1)
78+
}
79+
80+
/// Mask of every bit strictly below this tag's bit. Used to compute the
81+
/// byte offset of this tag's payload by summing the sizes of every
82+
/// present variant that packs before it.
83+
#[inline]
84+
pub(crate) const fn mask_below(self) -> u32 {
85+
self.bit() - 1
86+
}
87+
88+
/// Mask of every bit strictly above this tag's bit. Used when inserting
89+
/// or removing a payload: the bytes belonging to variants packed *after*
90+
/// this one have to shift to make room (insert) or close the gap (take).
91+
#[inline]
92+
pub(crate) const fn mask_above(self) -> u32 {
93+
!((self.bit() << 1) - 1)
94+
}
95+
}
96+
2697
/// Presence bitmap + length/capacity counters for the lazy-payload byte
2798
/// buffer. The buffer itself lives in `TaskStorage`'s allocation, just
2899
/// past the end of the `TaskStorageInner` head — `LazyTail` only carries the
@@ -70,16 +141,21 @@ impl LazyTail {
70141
/// The offset is the sum of `LAZY_PADDED_SIZE[k]` over every set bit `k`
71142
/// strictly below the bit for `tag` in `present`.
72143
#[inline]
73-
pub(crate) fn offset_of(&self, tag: u8) -> Option<usize> {
74-
let bit = 1u32 << (tag - 1);
75-
if self.present & bit == 0 {
144+
pub(crate) fn offset_of(&self, tag: Tag) -> Option<usize> {
145+
if self.present & tag.bit() == 0 {
76146
return None;
77147
}
78-
let mask_below = self.present & (bit - 1);
79-
Some(Self::sum_padded_sizes(mask_below))
148+
Some(Self::sum_padded_sizes(self.present & tag.mask_below()))
80149
}
81150

82151
/// Sum of `LAZY_PADDED_SIZE[k+1]` over every set bit `k` in `mask`.
152+
///
153+
/// Iterates set bits using the classic `mask &= mask - 1` trick (Daniel
154+
/// Lemire, "Iterating over set bits quickly":
155+
/// <https://lemire.me/blog/2018/02/21/iterating-over-set-bits-quickly/>),
156+
/// which runs in `popcount(mask)` steps rather than 32. The stdlib has
157+
/// `trailing_zeros` / `count_ones` but no built-in set-bit iterator;
158+
/// `bits &= bits - 1` is the canonical equivalent.
83159
#[inline]
84160
pub(crate) fn sum_padded_sizes(mut mask: u32) -> usize {
85161
let mut offset = 0usize;
@@ -94,8 +170,8 @@ impl LazyTail {
94170

95171
/// `true` iff the variant with `tag` has a payload in the tail.
96172
#[inline]
97-
pub(crate) fn has(&self, tag: u8) -> bool {
98-
self.present & (1u32 << (tag - 1)) != 0
173+
pub(crate) fn has(&self, tag: Tag) -> bool {
174+
self.present & tag.bit() != 0
99175
}
100176

101177
/// Get a typed reference to the payload for `tag`.
@@ -106,7 +182,7 @@ impl LazyTail {
106182
/// - `tail_base` must point to a buffer of at least `self.len` initialized bytes, with the
107183
/// payloads laid out as the install protocol writes them.
108184
#[inline]
109-
pub(crate) unsafe fn find<T>(&self, tag: u8, tail_base: *const u8) -> Option<&T> {
185+
pub(crate) unsafe fn find<T>(&self, tag: Tag, tail_base: *const u8) -> Option<&T> {
110186
let offset = self.offset_of(tag)?;
111187
// SAFETY: `offset < self.len <= cap`, so `tail_base.add(offset)` is
112188
// inside the buffer. The bytes at that offset were written as a `T`
@@ -120,7 +196,7 @@ impl LazyTail {
120196
/// Same as [`Self::find`]; caller has exclusive access via the `&mut`
121197
/// chain from `TaskStorage`.
122198
#[inline]
123-
pub(crate) unsafe fn find_mut<T>(&self, tag: u8, tail_base: *mut u8) -> Option<&mut T> {
199+
pub(crate) unsafe fn find_mut<T>(&self, tag: Tag, tail_base: *mut u8) -> Option<&mut T> {
124200
let offset = self.offset_of(tag)?;
125201
// SAFETY: see `find`.
126202
unsafe { Some(&mut *tail_base.add(offset).cast::<T>()) }
@@ -131,12 +207,10 @@ impl LazyTail {
131207
/// # Safety
132208
/// - `T` must match the payload type for `tag`.
133209
/// - `tail_base` must point to a buffer matching this `LazyTail`'s metadata.
134-
pub(crate) unsafe fn take<T>(&mut self, tag: u8, tail_base: *mut u8) -> Option<T> {
210+
pub(crate) unsafe fn take<T>(&mut self, tag: Tag, tail_base: *mut u8) -> Option<T> {
135211
let offset = self.offset_of(tag)?;
136-
let bit = 1u32 << (tag - 1);
137-
let payload_size = LAZY_PADDED_SIZE[tag as usize] as usize;
138-
let mask_above = self.present & !((bit << 1) - 1);
139-
let above_bytes = Self::sum_padded_sizes(mask_above);
212+
let payload_size = LAZY_PADDED_SIZE[tag.table_index()] as usize;
213+
let above_bytes = Self::sum_padded_sizes(self.present & tag.mask_above());
140214

141215
// SAFETY: `offset` is valid; we read the payload as `T` (caller
142216
// guarantees the type matches), then memmove the bytes above
@@ -155,29 +229,33 @@ impl LazyTail {
155229
};
156230

157231
self.len -= payload_size as u16;
158-
self.present &= !bit;
232+
self.present &= !tag.bit();
159233
Some(value)
160234
}
161235

162-
/// Install a payload for `tag`. Caller must have already ensured the
236+
/// Insert a payload for `tag`. Caller must have already ensured the
163237
/// buffer has enough capacity (`self.cap - self.len >= padded_size`).
164238
///
165239
/// # Safety
166240
/// - `T` must match the payload type for `tag`.
167-
/// - The variant must NOT already be present (caller should `take` it first to replace).
241+
/// - The variant must NOT already be present. To replace an existing payload, use
242+
/// [`Self::replace_in_place`] instead — it skips the shift work this method has to do.
168243
/// - `tail_base` must point to a buffer of at least `self.cap` bytes.
169-
pub(crate) unsafe fn install_unchecked<T>(&mut self, tag: u8, value: T, tail_base: *mut u8) {
170-
debug_assert!(!self.has(tag), "lazy variant tag {tag} already present");
171-
let bit = 1u32 << (tag - 1);
172-
let mask_below = self.present & (bit - 1);
244+
pub(crate) unsafe fn insert_unchecked<T>(&mut self, tag: Tag, value: T, tail_base: *mut u8) {
245+
debug_assert!(
246+
!self.has(tag),
247+
"lazy variant tag {} already present",
248+
tag.raw()
249+
);
250+
let mask_below = self.present & tag.mask_below();
173251
let mask_above = self.present & !mask_below;
174-
let payload_size = LAZY_PADDED_SIZE[tag as usize] as usize;
252+
let payload_size = LAZY_PADDED_SIZE[tag.table_index()] as usize;
175253
let offset = Self::sum_padded_sizes(mask_below);
176254
let above_bytes = Self::sum_padded_sizes(mask_above);
177255
let new_len = self.len as usize + payload_size;
178256
debug_assert!(
179257
new_len <= self.cap as usize,
180-
"install_unchecked exceeds capacity ({} > {}); caller must grow first",
258+
"insert_unchecked exceeds capacity ({} > {}); caller must grow first",
181259
new_len,
182260
self.cap,
183261
);
@@ -196,7 +274,7 @@ impl LazyTail {
196274
}
197275

198276
self.len = new_len as u16;
199-
self.present |= bit;
277+
self.present |= tag.bit();
200278
}
201279

202280
/// Replace the payload for `tag` in place (same tag, same size).
@@ -208,13 +286,14 @@ impl LazyTail {
208286
/// - `tail_base` must point to a buffer matching this `LazyTail`.
209287
pub(crate) unsafe fn replace_in_place<T>(
210288
&mut self,
211-
tag: u8,
289+
tag: Tag,
212290
value: T,
213291
tail_base: *mut u8,
214292
) -> T {
215293
debug_assert!(
216294
self.has(tag),
217-
"replace_in_place expects tag {tag} to be present"
295+
"replace_in_place expects tag {} to be present",
296+
tag.raw(),
218297
);
219298
// SAFETY: `has(tag)` => `offset_of(tag)` is `Some`. Caller guarantees
220299
// type match; same-tag replace has constant payload size so no
@@ -228,3 +307,135 @@ impl LazyTail {
228307
}
229308
}
230309
}
310+
311+
#[cfg(test)]
312+
mod tests {
313+
use super::*;
314+
use crate::backend::storage_schema::LAZY_N;
315+
316+
#[test]
317+
fn sum_padded_sizes_empty_mask_is_zero() {
318+
assert_eq!(LazyTail::sum_padded_sizes(0), 0);
319+
}
320+
321+
#[test]
322+
fn sum_padded_sizes_single_bit_matches_table() {
323+
// For each valid tag, a mask with only that one bit set should sum
324+
// to that tag's padded size — the loop runs exactly once.
325+
for tag_raw in 1..=LAZY_N {
326+
let tag = Tag::new(tag_raw);
327+
let expected = LAZY_PADDED_SIZE[tag.table_index()] as usize;
328+
assert_eq!(
329+
LazyTail::sum_padded_sizes(tag.bit()),
330+
expected,
331+
"single-bit sum for tag {tag_raw} should equal LAZY_PADDED_SIZE[{tag_raw}]",
332+
);
333+
}
334+
}
335+
336+
#[test]
337+
fn sum_padded_sizes_full_mask_sums_all_padded_sizes() {
338+
// Mask with every valid tag bit set should sum to the total of
339+
// `LAZY_PADDED_SIZE[1..=LAZY_N]`.
340+
let mut all_bits: u32 = 0;
341+
let mut expected: usize = 0;
342+
for tag_raw in 1..=LAZY_N {
343+
let tag = Tag::new(tag_raw);
344+
all_bits |= tag.bit();
345+
expected += LAZY_PADDED_SIZE[tag.table_index()] as usize;
346+
}
347+
assert_eq!(LazyTail::sum_padded_sizes(all_bits), expected);
348+
}
349+
350+
#[test]
351+
fn sum_padded_sizes_ignores_bits_above_lazy_n() {
352+
// `LAZY_PADDED_SIZE[i]` for `i > LAZY_N` is undefined (the array is
353+
// only `LAZY_N + 1` long), so bits above `LAZY_N` must never be
354+
// present in a real mask. This test pins the contract: with only
355+
// valid bits set, the sum equals the table sum.
356+
let mut mask: u32 = 0;
357+
let mut expected: usize = 0;
358+
// Use alternating bits to cover the popcount-iteration path.
359+
for tag_raw in (1..=LAZY_N).step_by(2) {
360+
let tag = Tag::new(tag_raw);
361+
mask |= tag.bit();
362+
expected += LAZY_PADDED_SIZE[tag.table_index()] as usize;
363+
}
364+
assert_eq!(LazyTail::sum_padded_sizes(mask), expected);
365+
}
366+
367+
#[test]
368+
fn offset_of_for_present_tag_matches_sum_of_padded_sizes_below() {
369+
// For a tail where two tags below `T` are present, the offset of
370+
// `T`'s payload must equal the sum of those two padded sizes.
371+
// Skip if the schema is too small to set up the scenario.
372+
if LAZY_N < 3 {
373+
return;
374+
}
375+
let t1 = Tag::new(1);
376+
let t2 = Tag::new(2);
377+
let t3 = Tag::new(3);
378+
let mut tail = LazyTail::default();
379+
tail.present = t1.bit() | t2.bit() | t3.bit();
380+
assert_eq!(tail.offset_of(t1), Some(0));
381+
assert_eq!(
382+
tail.offset_of(t2),
383+
Some(LAZY_PADDED_SIZE[t1.table_index()] as usize),
384+
);
385+
assert_eq!(
386+
tail.offset_of(t3),
387+
Some(
388+
LAZY_PADDED_SIZE[t1.table_index()] as usize
389+
+ LAZY_PADDED_SIZE[t2.table_index()] as usize,
390+
),
391+
);
392+
}
393+
394+
#[test]
395+
fn offset_of_for_absent_tag_returns_none() {
396+
let t1 = Tag::new(1);
397+
let t2 = Tag::new(2);
398+
let mut tail = LazyTail::default();
399+
tail.present = t1.bit();
400+
assert_eq!(tail.offset_of(t1), Some(0));
401+
assert_eq!(tail.offset_of(t2), None);
402+
}
403+
404+
#[test]
405+
fn tag_masks_partition_the_other_bits() {
406+
// For any tag, `mask_below | bit | mask_above` covers every bit in
407+
// a u32 with no overlap. This is the structural invariant the
408+
// insert/take/offset code relies on.
409+
for tag_raw in 1..=LAZY_N {
410+
let tag = Tag::new(tag_raw);
411+
let union = tag.mask_below() | tag.bit() | tag.mask_above();
412+
assert_eq!(
413+
union,
414+
u32::MAX,
415+
"masks must cover every bit for tag {tag_raw}"
416+
);
417+
assert_eq!(
418+
tag.mask_below() & tag.bit(),
419+
0,
420+
"mask_below must not overlap bit for tag {tag_raw}",
421+
);
422+
assert_eq!(
423+
tag.mask_above() & tag.bit(),
424+
0,
425+
"mask_above must not overlap bit for tag {tag_raw}",
426+
);
427+
assert_eq!(
428+
tag.mask_below() & tag.mask_above(),
429+
0,
430+
"mask_below and mask_above must not overlap for tag {tag_raw}",
431+
);
432+
}
433+
}
434+
435+
#[test]
436+
fn option_tag_niches_to_one_byte() {
437+
// Tag wraps `NonZeroU8`, so `Option<Tag>` should reuse the zero
438+
// value as the `None` discriminant.
439+
assert_eq!(std::mem::size_of::<Option<Tag>>(), 1);
440+
}
441+
}

turbopack/crates/turbo-tasks-backend/src/backend/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3755,7 +3755,7 @@ fn encode_task_data(
37553755
data.encode(category, &mut encoder)?;
37563756

37573757
if cfg!(feature = "verify_serialization") {
3758-
crate::backend::task_storage::TaskStorage::new()
3758+
TaskStorage::new()
37593759
.decode(
37603760
category,
37613761
&mut new_turbo_bincode_decoder(&scratch_buffer[..]),

0 commit comments

Comments
 (0)