Skip to content

DiskFileSystem: store Effect full_path as Arc<Path>#94332

Open
sokra wants to merge 1 commit into
vercel:canaryfrom
sokra:sokra/arc-path
Open

DiskFileSystem: store Effect full_path as Arc<Path>#94332
sokra wants to merge 1 commit into
vercel:canaryfrom
sokra:sokra/arc-path

Conversation

@sokra
Copy link
Copy Markdown
Member

@sokra sokra commented Jun 2, 2026

What?

In the DiskFileSystem write and write_link implementations (turbopack/crates/turbo-tasks-fs/src/lib.rs), change the full_path field on the WriteEffect and WriteLinkEffect structs from Arc<PathBuf> to Arc<Path>.

Why?

Arc<PathBuf> stores two heap allocations and an extra indirection: the Arc allocation holds a PathBuf (Vec<u8>-like), which in turn points to the actual path bytes. Arc<Path> is a thin/wide-pointer to a single allocation containing the path bytes inline next to the Arc header, which is both smaller (no separate PathBuf capacity field) and removes one level of pointer chasing on every access.

These effects are cloned per write side-effect and live until the effect is applied, so the saved allocation/indirection is a small but free win.

How?

  • Change the full_path field type on WriteEffect and WriteLinkEffect from Arc<PathBuf> to Arc<Path>.
  • Update the two construction sites from Arc::new(full_path) to Arc::from(full_path), which uses the standard library's From<PathBuf> for Arc<Path> impl to move the path bytes directly into a single Arc<Path> allocation.

No call sites needed updating: all existing uses (validate_path_length(&self.full_path), self.inner.invalidate_from_write(&self.full_path), self.full_path.as_os_str()) already only need &Path, which is reached via Deref from both Arc<PathBuf> and Arc<Path>.

Verified locally with cargo check -p turbo-tasks-fs and cargo clippy -p turbo-tasks-fs --all-targets.

Switches the `full_path` field in `WriteEffect` and `WriteLinkEffect`
from `Arc<PathBuf>` to `Arc<Path>`, avoiding the extra indirection and
allocation of a `PathBuf` inside the `Arc`.

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Tobias Koppers <sokra@users.noreply.github.com>
@sokra sokra requested a review from bgw June 2, 2026 08:45
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 2, 2026

Failing test suites

Commit: 06948a9 | About building and testing Next.js

pnpm test-start-turbo test/e2e/app-dir/navigation/navigation.test.ts (turbopack) (job)

  • app dir - navigation > hash > should scroll to the specified hash (DD)
Expand output

● app dir - navigation › hash › should scroll to the specified hash

expect(received).toBe(expected) // Object.is equality

Expected: false
Received: true

  197 |           url.includes('with-query-param')
  198 |         )
> 199 |         expect(hasQueryParamRscRequest).toBe(false)
      |                                         ^
  200 |       }
  201 |
  202 |       await checkLink('query-param', 2284)

  at Object.toBe (e2e/app-dir/navigation/navigation.test.ts:199:41)

Other failing CI jobs

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 2, 2026

Stats skipped

Commit: 06948a9
View workflow run

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants