Skip to content

fix: remove unused fields in rvstate#2887

Open
mansur20478 wants to merge 1 commit into
feat/rvr-support-rv64-riscvfrom
feat/rvr-remove-dead-rvstate-fields
Open

fix: remove unused fields in rvstate#2887
mansur20478 wants to merge 1 commit into
feat/rvr-support-rv64-riscvfrom
feat/rvr-remove-dead-rvstate-fields

Conversation

@mansur20478

Copy link
Copy Markdown

removes unused reservation_addr, reservation_valid, brk, start_brk, csrs fields from RvState.

@mansur20478 mansur20478 self-assigned this Jun 12, 2026
@github-actions

Copy link
Copy Markdown
group app.proof_time_ms app.cycles leaf.proof_time_ms
fibonacci 1,645 4,000,051 521
keccak 16,359 14,365,133 3,052
sha2_bench 10,447 11,167,961 1,940
regex 1,545 4,090,656 430
ecrecover 486 112,210 307
pairing 632 592,827 308
kitchen_sink 3,915 1,979,971 860

Note: cells_used metrics omitted because CUDA tracegen does not expose unpadded trace heights.

Commit: 4813b4b

Benchmark Workflow

@mansur20478 mansur20478 marked this pull request as ready for review June 12, 2026 19:03
@mansur20478 mansur20478 requested a review from shuklaayush June 12, 2026 19:03
@shuklaayush

Copy link
Copy Markdown
Collaborator

@claude @codex there might've been a performance reason why we added this. do we not need it anymore?

    /// Alignment padding.
    _pad0: u8,

@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Claude finished @shuklaayush's task in 1m 48s —— View job


Investigating _pad0 alignment padding field

  • Read the current state of RvState and the PR diff
  • Investigate git history for why _pad0 was added
  • Analyze whether the padding is still needed for performance/alignment

Answer

The _pad0: u8 field is not needed anymore and is safe to remove. It was explicit alignment padding, not a performance optimization.

Why it existed (old layout):

In the old struct, the field sequence around _pad0 was:

reservation_addr: u32    // offset 280, size 4
reservation_valid: u8    // offset 284, size 1
has_exited: u8           // offset 285, size 1
exit_code: u8            // offset 286, size 1
_pad0: u8                // offset 287, size 1  ← explicit pad
brk: u32                 // offset 288, size 4  ← needs 4-byte alignment
start_brk: u32           // offset 292, size 4
memory: *mut u8          // offset 296, size 8

After three consecutive u8 fields (reservation_valid, has_exited, exit_code), we're at offset 287. The next field brk: u32 needs 4-byte alignment (offset 288). _pad0 was an explicit padding byte to keep the Rust repr(C) layout in exact sync with the handwritten C struct, ensuring no implicit padding surprises in the FFI boundary.

Why it's safe to remove (new layout):

With reservation_addr, reservation_valid, brk, start_brk, and csrs all gone, the layout becomes:

target_instret: u64      // offset 272, size 8
has_exited: u8           // offset 280, size 1
exit_code: u8            // offset 281, size 1
// (6 bytes implicit padding — repr(C) aligns to next field's requirement)
memory: *mut u8          // offset 288, size 8  ← needs 8-byte alignment

The compiler handles the padding between exit_code and memory automatically in both repr(C) Rust and C. Both the Rust struct and C struct agree on the layout without any explicit padding field.

tl;dr: _pad0 existed to align the now-removed brk: u32 field. No performance concern — just FFI layout matching.


@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. What shall we delve into next?

Reviewed commit: 4813b4b58b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@shuklaayush shuklaayush left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. let's merge this after the parent is merged

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