Skip to content

Remove FLATTEN_MAPS thread local, store it in ZalsaLocal instead#1124

Draft
Veykril wants to merge 1 commit into
salsa-rs:masterfrom
Veykril:lukaswirth/push-nltxvtsyqlrr
Draft

Remove FLATTEN_MAPS thread local, store it in ZalsaLocal instead#1124
Veykril wants to merge 1 commit into
salsa-rs:masterfrom
Veykril:lukaswirth/push-nltxvtsyqlrr

Conversation

@Veykril

@Veykril Veykril commented Jun 10, 2026

Copy link
Copy Markdown
Member

No description provided.

@netlify

netlify Bot commented Jun 10, 2026

Copy link
Copy Markdown

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit 0ff0ffd
🔍 Latest deploy log https://app.netlify.com/projects/salsa-rs/deploys/6a2913627c9ea90008b884ad

@codspeed-hq

codspeed-hq Bot commented Jun 10, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 4.21%

❌ 1 regressed benchmark
✅ 7 untouched benchmarks
🗄️ 18 archived benchmarks run1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Memory converge_diverge_nested 12.2 KB 12.7 KB -4.21%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing Veykril:lukaswirth/push-nltxvtsyqlrr (0ff0ffd) with master (f6325bb)

Open in CodSpeed

Footnotes

  1. 18 benchmarks were run, but are now archived. If they were deleted in another branch, consider rebasing to remove them from the report. Instead if they were added back, click here to restore them.

@Veykril Veykril force-pushed the lukaswirth/push-nltxvtsyqlrr branch from 9fa55ef to 0ff0ffd Compare June 10, 2026 07:33
@Veykril Veykril marked this pull request as ready for review June 10, 2026 07:34
@MichaReiser

Copy link
Copy Markdown
Contributor

Can you say more what the benefits of this change are? Is it that the memory gets released when dropping the ZalsaLocal instance over retaining it forever?

I'd have to measure this change on ty but I'm a little concerned about a typical multithreaded use case where we create a new rayon task for each file, and each task uses its own Db clone (its own ZalsaLocal)

@Veykril

Veykril commented Jun 13, 2026

Copy link
Copy Markdown
Member Author

I generally favor having less thread locals, given we already have the thread local state stashed away in ZalsaLocal. Generally I wanted to check the effect of the benching numbers here. You do bring up a good point regarding the fact that ZalsaLocal's tend to get dropped and re-created frequently though (sounds like another thing we might want to have a benchmark for in fact).

Gonna put this in draft again until I can give that some more thought.

@Veykril Veykril marked this pull request as draft June 13, 2026 10:44
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