Skip to content

chore(bench): optimization of compile time for hlapi common bench#3382

Open
SouchonTheo wants to merge 1 commit intomainfrom
ts/bench/optimisation-hlapi-compile-time
Open

chore(bench): optimization of compile time for hlapi common bench#3382
SouchonTheo wants to merge 1 commit intomainfrom
ts/bench/optimisation-hlapi-compile-time

Conversation

@SouchonTheo
Copy link
Contributor

@SouchonTheo SouchonTheo commented Mar 10, 2026

optimization regarding compile time
went from 4:30 to 2:50 because we wrap the closure

This idea come from the fact that closure generate unique stuff

I ask claude if a workaround is possible and only wrapping was enough
tried it and it was working like a charm


This change is Reviewable

@SouchonTheo SouchonTheo requested a review from soonum as a code owner March 10, 2026 15:35
@cla-bot cla-bot bot added the cla-signed label Mar 10, 2026
Copy link
Contributor

@soonum soonum left a comment

Choose a reason for hiding this comment

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

Nice build optimization !
Wdyt @IceTDrinker ?

@IceTDrinker
Copy link
Member

Nice build optimization ! Wdyt @IceTDrinker ?

Question is do we want to support closures ?

otherwise I think I would update the various Op structs like BinaryOp to force a fn instead of Fn

fn is a function, Fn is a trait allowing some type of call, I believe that if closures are not doing crazy stuff they can be coerced into fn (as the added wrapper functions do)

So the current solution works and allows to still use closures, if we don't care about closures, I would remove the wrapping and change what we accept in the various Op structs, since it seems it's mostly for auto generated code and all closures get wrapped ?

@IceTDrinker
Copy link
Member

e.g.

impl<FheType, FheRhsType, F, R, EncryptLhsType, EncryptRhsType> BenchmarkOp<FheType>
    for BinaryOp<F, EncryptLhsType, EncryptRhsType, FheRhsType>
where
    F: Fn(&FheType, &FheRhsType) -> R,
    R: BenchWait,

the F could be replaced maybe ?

unclear I checked quickly

Copy link
Contributor Author

@SouchonTheo SouchonTheo left a comment

Choose a reason for hiding this comment

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

sounds a good idea
I can make a test it would not take so long to do

@SouchonTheo made 1 comment.
Reviewable status: 0 of 3 files reviewed, all discussions resolved.

@SouchonTheo SouchonTheo force-pushed the ts/bench/optimisation-hlapi-compile-time branch from cb78217 to e49b07c Compare March 11, 2026 15:29
@SouchonTheo SouchonTheo force-pushed the ts/bench/optimisation-hlapi-compile-time branch from e49b07c to 650a491 Compare March 11, 2026 15:30
Copy link
Contributor Author

@SouchonTheo SouchonTheo left a comment

Choose a reason for hiding this comment

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

the solution was even easier
We only needed to infer the type in benchmark_op
with that we avoid a lot of monomorphization issue

@SouchonTheo made 1 comment.
Reviewable status: 0 of 3 files reviewed, all discussions resolved.

@IceTDrinker
Copy link
Member

the solution was even easier We only needed to infer the type in benchmark_op with that we avoid a lot of monomorphization issue

all good for the build ? no issues with the "restriction" ?

still fast to build ?

Copy link
Contributor Author

@SouchonTheo SouchonTheo left a comment

Choose a reason for hiding this comment

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

same timing

@SouchonTheo made 1 comment.
Reviewable status: 0 of 4 files reviewed, all discussions resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants