Skip to content

Use alloca to address memory layout effects#744

Closed
PSeitz wants to merge 4 commits intobheisler:masterfrom
PSeitz:randomize_mem
Closed

Use alloca to address memory layout effects#744
PSeitz wants to merge 4 commits intobheisler:masterfrom
PSeitz:randomize_mem

Conversation

@PSeitz
Copy link
Copy Markdown
Contributor

@PSeitz PSeitz commented Nov 22, 2023

  • replace collect with Vec::reserve
    the iter collect pollutes the call stack (10 levels of calls)
  • offset tests with alloca
    As mentioned in Eliminate memory layout bias when measuring (with LLVM stabilizer) #334, randomness in memory layout has a huge impact on the bench results. When I benchmark with criterion it's not uncommon to see effects of +-30%. Tests seem much more stable with alloca, there's still random 6% jumps in one test I've been running.

Adresses #334

Copy link
Copy Markdown
Contributor

@samueltardieu samueltardieu left a comment

Choose a reason for hiding this comment

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

I like this changes. They should give better stability by increasing testing time variability.

Comment thread src/routine.rs

b.iters = b.iters.wrapping_mul(2);
b.iters = b.iters.min(64); // To make sure we offset the test at least with 0-64 bytes
// wit alloca
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: typo ("wit" -> "with"). Also, the comment is typically placed above the line, not on the side.

Comment thread src/routine.rs
Comment on lines +252 to +253
stack_alloc, /* how much bytes we want to allocate */
|_memory: &mut [core::mem::MaybeUninit<u8>] /* dynamically stack allocated slice itself */| {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe stack_alloc should be inlined here, to prevent getting a warning on platforms not selected by the cfg attribute. Using a name such as _shifting_stack_space or similar instead of _memory would be self-documenting.

@FilipAndersson245
Copy link
Copy Markdown

Seems quite straightforward and if this helps in reducing random changes between runs I'm all for this.

@waywardmonkeys
Copy link
Copy Markdown
Contributor

@PSeitz Thanks for your contribution! Are you interested in updating this PR for the review comments or should we take it on?

@berkus
Copy link
Copy Markdown
Contributor

berkus commented Nov 29, 2025

Merged, can be closed.

@PSeitz PSeitz closed this Nov 30, 2025
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.

5 participants