Skip to content

Conversation

Veykril
Copy link
Member

@Veykril Veykril commented Mar 1, 2025

No description provided.

Copy link

netlify bot commented Mar 1, 2025

Deploy Preview for salsa-rs canceled.

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

Copy link

codspeed-hq bot commented Mar 1, 2025

CodSpeed Performance Report

Merging #742 will not alter performance

Comparing Veykril:veykril/push-rmymmuwytuzz (b9379b0) with master (8fb172b)

Summary

✅ 12 untouched benchmarks

@Veykril Veykril changed the title Adjust safety argument of par_map Clean up par_map a bit Mar 5, 2025
@Veykril Veykril changed the title Clean up par_map a bit refactor: Clean up par_map a bit Mar 10, 2025
@Veykril Veykril force-pushed the veykril/push-rmymmuwytuzz branch from 0b52a49 to e908a34 Compare March 15, 2025 11:06
@MichaReiser MichaReiser requested a review from davidbarsky March 17, 2025 09:29
@Veykril Veykril force-pushed the veykril/push-rmymmuwytuzz branch from e908a34 to 4107aa6 Compare March 20, 2025 13:08
@Veykril Veykril force-pushed the veykril/push-rmymmuwytuzz branch 2 times, most recently from c525227 to 1f6f19b Compare March 22, 2025 11:58
@Veykril
Copy link
Member Author

Veykril commented Mar 22, 2025

@davidbarsky bad news, the current impl of par_map is unsound, we are smuggling the database reference across threads before calling clone on it.

@Veykril Veykril force-pushed the veykril/push-rmymmuwytuzz branch from 1f6f19b to 6fd03f3 Compare March 22, 2025 12:00
@Veykril Veykril force-pushed the veykril/push-rmymmuwytuzz branch from 6fd03f3 to a8e9ed7 Compare March 22, 2025 13:11
@davidbarsky
Copy link
Contributor

@davidbarsky bad news, the current impl of par_map is unsound, we are smuggling the database reference across threads before calling clone on it.

Crap, I'm hoping this is fixable.

@Veykril
Copy link
Member Author

Veykril commented Mar 22, 2025

It is, we just need to eagerly fork_db, sending a Box<dyn Database> is fine, but &dyn Database is not given its not Send

@Veykril Veykril force-pushed the veykril/push-rmymmuwytuzz branch from fe2d474 to bc8e406 Compare March 24, 2025 06:01
@Veykril
Copy link
Member Author

Veykril commented Mar 24, 2025

Fixed the issue and added some more tests for the other new APIs

@davidbarsky
Copy link
Contributor

Changes look good to me. Should we add a par_for_each function as well?

@Veykril Veykril added this pull request to the merge queue Mar 24, 2025
@Veykril
Copy link
Member Author

Veykril commented Mar 24, 2025

We could

Merged via the queue into salsa-rs:master with commit 5ee3bdd Mar 24, 2025
11 checks passed
@Veykril Veykril deleted the veykril/push-rmymmuwytuzz branch March 24, 2025 19:55
@github-actions github-actions bot mentioned this pull request Mar 24, 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