Skip to content

Conversation

Veykril
Copy link
Member

@Veykril Veykril commented Mar 15, 2025

The number of cycle participants is generally low so a hashset likely has more overhead compared to a simple vec for such small numbers of elements.

Copy link

netlify bot commented Mar 15, 2025

Deploy Preview for salsa-rs canceled.

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

Copy link

codspeed-hq bot commented Mar 15, 2025

CodSpeed Performance Report

Merging #760 will improve performances by 5.54%

Comparing Veykril:veykril/push-myoqmkxmwwrm (5b718d7) with master (1bbf4c2)

Summary

⚡ 4 improvements
✅ 8 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
amortized[Input] 3.6 µs 3.5 µs +5.12%
amortized[InternedInput] 3.5 µs 3.3 µs +5.3%
amortized[SupertypeInput] 4.3 µs 4.1 µs +5.03%
converge_diverge 157.7 µs 149.4 µs +5.54%

@Veykril Veykril force-pushed the veykril/push-myoqmkxmwwrm branch from 90345f4 to de19b2e Compare March 15, 2025 12:51
@Veykril Veykril changed the title Use a Vec for CycleHeads perf: Use a Vec for CycleHeads Mar 15, 2025
@Veykril Veykril marked this pull request as ready for review March 15, 2025 12:52
The number of cycle participants is generally low so a hashset likely has more overhead compared to a simple vec
@Veykril Veykril force-pushed the veykril/push-myoqmkxmwwrm branch from de19b2e to b4929f1 Compare March 15, 2025 13:04
@Veykril Veykril changed the title perf: Use a Vec for CycleHeads perf: Use a ThinVec for CycleHeads Mar 15, 2025
@Veykril Veykril force-pushed the veykril/push-myoqmkxmwwrm branch 4 times, most recently from d5efeed to f96b9cc Compare March 15, 2025 13:23
@Veykril Veykril changed the title perf: Use a ThinVec for CycleHeads perf: Use a Vec for CycleHeads Mar 15, 2025
@Veykril Veykril force-pushed the veykril/push-myoqmkxmwwrm branch from f96b9cc to efec0f9 Compare March 15, 2025 13:27
@MichaReiser MichaReiser requested a review from carljm March 15, 2025 15:11
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

pub(crate) struct CycleHeadsIter<'a>(
Option<std::collections::hash_set::Iter<'a, DatabaseKeyIndex>>,
);
pub struct CycleHeadsIter<'a>(std::slice::Iter<'a, DatabaseKeyIndex>);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if you consider this an improvement, but we can eliminate CycleHeadsIter and implement IntoIterator for &'a CycleHeads like so:

impl<'a> std::iter::IntoIterator for &'a CycleHeads {
    type Item = DatabaseKeyIndex;
    type IntoIter = std::iter::Copied<std::slice::Iter<'a, DatabaseKeyIndex>>;

    fn into_iter(self) -> Self::IntoIter {
        self.0
            .as_ref()
            .map(|heads| heads.iter())
            .unwrap_or_default()
            .copied()
    }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

This causes new[InternedInput] to regress due to a report_tracked_read becoming more expensive, so something optimizes differently there 😕 Will drop our Extend impl as we can optimize around knowing the concrete iterator anyways.

@Veykril Veykril force-pushed the veykril/push-myoqmkxmwwrm branch from 7e21992 to 79505fc Compare March 15, 2025 16:51
@Veykril Veykril enabled auto-merge March 15, 2025 16:51
@Veykril Veykril disabled auto-merge March 15, 2025 16:55
@Veykril Veykril force-pushed the veykril/push-myoqmkxmwwrm branch 4 times, most recently from 2f37075 to 7e21992 Compare March 15, 2025 17:05
@Veykril Veykril force-pushed the veykril/push-myoqmkxmwwrm branch 3 times, most recently from 5a5a6d0 to 973c711 Compare March 15, 2025 17:24
@Veykril Veykril force-pushed the veykril/push-myoqmkxmwwrm branch from 973c711 to 5b718d7 Compare March 15, 2025 17:36
@Veykril Veykril enabled auto-merge March 15, 2025 17:39
@Veykril Veykril added this pull request to the merge queue Mar 15, 2025
Merged via the queue into salsa-rs:master with commit 407e9e0 Mar 15, 2025
11 checks passed
@Veykril Veykril deleted the veykril/push-myoqmkxmwwrm branch March 15, 2025 17:54
@github-actions github-actions bot mentioned this pull request Mar 15, 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