-
Notifications
You must be signed in to change notification settings - Fork 315
refactor(core): simplify PlanMap #3115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
IceTDrinker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be worth checking there is no deadlock possible, I seem to recall the nasty type was required to avoid them, also this PR only updates the NTT one
fdc5eeb to
a2b802e
Compare
|
The 2 commits should be reviewed separately We may only keep the first one |
IceTDrinker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just as an FYI, if we go ahead with this, I would prefer it for Q1 26 (because it's a change at the bottom of the stack with potential weird bugs due to threading) so not for 1.5
Reviewable status: 0 of 5 files reviewed, all discussions resolved
Let's only keep the first commit for now then |
|
@mayeul-zama if you can rebase on main and launch tests with the approved label to see if we hit deadlocks |
a2b802e to
4a4aa84
Compare
4a4aa84 to
6110942
Compare
IceTDrinker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I had time to read the PR, it looks like it should work for our usage and is indeed a nice simplification
a few small comments
@IceTDrinker reviewed 5 files and all commit messages, and made 6 comments.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @mayeul-zama).
tfhe/src/core_crypto/commons/math/ntt/ntt64.rs line 27 at r3 (raw file):
// Key is (polynomial size, modulus). type PlanMap = crate::core_crypto::commons::plan::PlanMap<(usize, u64), Plan>;
import PlanMap directly from the same location of new_from_plan_map, or even create your own wrapper type and put new_from_plan_map on it (and rename it perhaps as indicated in another file)
tfhe/src/core_crypto/fft_impl/fft128/math/fft/mod.rs line 13 at r3 (raw file):
#[derive(Clone)] pub(crate) struct PlanWrapper(Plan);
is that PlanWrapper doing anything ? Now that I'm looking seems a bit odd, I don't see it for the other plans
tfhe/src/core_crypto/fft_impl/fft128/math/fft/mod.rs line 49 at r3 (raw file):
} type PlanMap = crate::core_crypto::commons::plan::PlanMap<usize, PlanWrapper>;
import PlanMap directly
tfhe/src/core_crypto/commons/plan.rs line 7 at r2 (raw file):
pub type PlanMap<Key, Value> = RwLock<HashMap<Key, Arc<Value>>>; pub fn new_from_plan_map<Key: Eq + Hash + Copy, Value>(
get_or_init ? looks very much like the OnceLock function with the same name
tfhe/src/core_crypto/fft_impl/fft64/math/fft/mod.rs line 105 at r3 (raw file):
} type PlanMap = crate::core_crypto::commons::plan::PlanMap<usize, (Twisties, Plan)>;
import directly
|
not sure which PR between this one and #3116 should be the one to review ? this one looked ok but the other one has more changed files but conflicts |
entries of the
HashMapare always initialized so we don't need them to be wrapped inOnceLockThis change is