Skip to content

refactor: Move verified_final from Memo into QueryRevisions#769

Merged
Veykril merged 2 commits intosalsa-rs:masterfrom
Veykril:veykril/push-xoyllxvompzq
Mar 22, 2025
Merged

refactor: Move verified_final from Memo into QueryRevisions#769
Veykril merged 2 commits intosalsa-rs:masterfrom
Veykril:veykril/push-xoyllxvompzq

Conversation

@Veykril
Copy link
Member

@Veykril Veykril commented Mar 20, 2025

This saves an entire usize per function Memo

@netlify
Copy link

netlify bot commented Mar 20, 2025

Deploy Preview for salsa-rs canceled.

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

@codspeed-hq
Copy link

codspeed-hq bot commented Mar 20, 2025

CodSpeed Performance Report

Merging #769 will improve performances by 3.35%

Comparing Veykril:veykril/push-xoyllxvompzq (9eef15b) with master (6cfe2aa)

Summary

⚡ 1 improvements
✅ 11 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
accumulator 4.5 ms 4.3 ms +3.35%

@MichaReiser MichaReiser requested a review from carljm March 20, 2025 13:29
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.

Nice perf improvement! I considered trying this but thought it better to keep QueryRevisions immutable; but now that I look at this PR, I don't really see any strong reason for that.

Why does it save memory? Is it just a matter of the padding/alignment?

@Veykril Veykril force-pushed the veykril/push-xoyllxvompzq branch from e6e8529 to de8cb1d Compare March 21, 2025 05:28
@Veykril
Copy link
Member Author

Veykril commented Mar 21, 2025

Why does it save memory? Is it just a matter of the padding/alignment?

Yes QueryRevisions has some free padding this can fit in, where as Memo does not usually (it depends on the V type in it).

@Veykril Veykril force-pushed the veykril/push-xoyllxvompzq branch from de8cb1d to 5452c6b Compare March 21, 2025 05:35
@Veykril
Copy link
Member Author

Veykril commented Mar 21, 2025

Rebased and now the numbers are randomly red in report_tracked_read again 😩 I fear we might be within limits of where cache misses become very obvious in the benchmarks, given report_tracked_read goes through two heap allocs (querystack -> edge indexset).

Thus I'd personally say we should discard the regression here, I don't see it as meaningful. (and we do have to keep an eye out for memory right now, as r-a has had some bad regressions after all)

@Veykril Veykril force-pushed the veykril/push-xoyllxvompzq branch 2 times, most recently from 129391b to 22c0903 Compare March 21, 2025 05:48
This saves an entire usize per function `Memo`
@Veykril Veykril force-pushed the veykril/push-xoyllxvompzq branch from 22c0903 to 021d93e Compare March 21, 2025 05:52
@Veykril Veykril added this pull request to the merge queue Mar 22, 2025
Merged via the queue into salsa-rs:master with commit 3c555a2 Mar 22, 2025
11 checks passed
@Veykril Veykril deleted the veykril/push-xoyllxvompzq branch March 22, 2025 09:10
@github-actions github-actions bot mentioned this pull request Mar 22, 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