Skip to content

Conversation

Veykril
Copy link
Member

@Veykril Veykril commented Mar 22, 2025

Removes an unnecessary heap allocation when recording a dependency edge

Copy link

netlify bot commented Mar 22, 2025

Deploy Preview for salsa-rs canceled.

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

Copy link

codspeed-hq bot commented Mar 22, 2025

CodSpeed Performance Report

Merging #773 will not alter performance

Comparing Veykril:veykril/push-nmzmtvrqwwuv (b12d5f5) with master (296a8c7)

Summary

✅ 12 untouched benchmarks

@Veykril Veykril marked this pull request as draft March 22, 2025 20:47
@Veykril
Copy link
Member Author

Veykril commented Mar 22, 2025

Okay I think this does confirm that the benches are really sensitive to allocation changes. This should not regress performance whatsoever.

@Veykril Veykril marked this pull request as ready for review March 22, 2025 20:49
@Veykril Veykril changed the title refactor: Keep edge condvar on stack instead of allocating it in an Arc refactor: Keep edge condvar on the stack Mar 22, 2025
@Veykril Veykril force-pushed the veykril/push-nmzmtvrqwwuv branch from c0081cd to 83b1ce2 Compare March 26, 2025 06:29
@Veykril Veykril force-pushed the veykril/push-nmzmtvrqwwuv branch 2 times, most recently from 00f2614 to b12d5f5 Compare April 5, 2025 08:30
Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

I'm a bit concerned about the amount of unsafe code in salsa. But maybe that's just me because I'm not used to writing unsafe code. Avoiding an allocation in add_edge certainly is nice. But it's not entirely clear when we think that using unsafe is justified

@Veykril Veykril added this pull request to the merge queue Apr 5, 2025
Merged via the queue into salsa-rs:master with commit 2253999 Apr 5, 2025
11 checks passed
@Veykril Veykril deleted the veykril/push-nmzmtvrqwwuv branch April 5, 2025 18:51
@github-actions github-actions bot mentioned this pull request Apr 3, 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.

2 participants