Skip to content

Update boxcar #696

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 20, 2025
Merged

Conversation

ibraheemdev
Copy link
Contributor

It looks like the reason for the regression in #688 might have been simpler than I thought and come down to some poor codegen when allocating new boxcar buckets (see ibraheemdev/boxcar#28). Hoping CodSpeed also picks up these changes.

Copy link

netlify bot commented Feb 19, 2025

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit 1c301d2
🔍 Latest deploy log https://app.netlify.com/sites/salsa-rs/deploys/67dc1958657cd20008dc2b1e

Copy link

codspeed-hq bot commented Feb 19, 2025

CodSpeed Performance Report

Merging #696 will improve performances by 3.09%

Comparing ibraheemdev:boxcar-alloc-zeroed (1c301d2) with master (7fe054e)

Summary

⚡ 1 improvements
✅ 11 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
new[InternedInput] 5.8 µs 5.7 µs +3.09%

@ibraheemdev
Copy link
Contributor Author

ibraheemdev commented Feb 19, 2025

The CodSpeed regression is interesting.. I see a very clear improvement locally.

@davidbarsky
Copy link
Contributor

Maybe one way to handle the noise from codespeed is to look at instruction counts before/after the change?

@ibraheemdev ibraheemdev force-pushed the boxcar-alloc-zeroed branch from ec6b6c5 to 27cf84a Compare March 20, 2025 13:25
@ibraheemdev ibraheemdev force-pushed the boxcar-alloc-zeroed branch from 27cf84a to 1c301d2 Compare March 20, 2025 13:34
@ibraheemdev
Copy link
Contributor Author

ibraheemdev commented Mar 20, 2025

Looks like this is a win across the board now. I suspect that's due to #725. I'm not sure what allocator CodSpeed was using previously, but the change should be a clear improvement.

@ibraheemdev ibraheemdev marked this pull request as ready for review March 20, 2025 13:35
@MichaReiser MichaReiser added this pull request to the merge queue Mar 20, 2025
Merged via the queue into salsa-rs:master with commit e826b5a Mar 20, 2025
11 checks passed
@github-actions github-actions bot mentioned this pull request Mar 20, 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.

4 participants