Skip to content

Revert "remove borrow checks from ZalsaLocal" #950

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ibraheemdev
Copy link
Member

This is showing up as a 7% regression on master?

Copy link

netlify bot commented Aug 1, 2025

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit b9ae30d
🔍 Latest deploy log https://app.netlify.com/projects/salsa-rs/deploys/688ce62a72242800084a26d5

Copy link

codspeed-hq bot commented Aug 1, 2025

CodSpeed Performance Report

Merging #950 will improve performances by 10.42%

Comparing ibraheemdev:ibraheem/revert-unchecked-zalsa-local (b9ae30d) with master (0ca4f4f)

Summary

⚡ 2 improvements
✅ 10 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
amortized[InternedInput] 2.5 µs 2.2 µs +10.42%
new[InternedInput] 4.3 µs 4.1 µs +4.99%

@Veykril
Copy link
Member

Veykril commented Aug 2, 2025

That is odd ...

@Veykril
Copy link
Member

Veykril commented Aug 2, 2025

jeamlloc appears in some diffs, so this might be allocator noise? Though this PR shouldn't be touching things in a way to affect allocations?

Looking at some of the other diffs there they really do not seem related, like dyn_memos getting slower. The only part there that can get slower is the memory access which again points to allocation noise imo.

Can you reproduce this regression on ty? If not I would say we should keep the changes.

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