Skip to content
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

perf: allocate less in test engine #331

Merged
merged 2 commits into from Jul 6, 2022
Merged

perf: allocate less in test engine #331

merged 2 commits into from Jul 6, 2022

Conversation

ivokub
Copy link
Collaborator

@ivokub ivokub commented Jun 23, 2022

At every API call utils.FromInterface method was called. This method always
allocates new *big.Int, which is very stressful for the runtime. Instead, we
first check if the variable is already *big.Int and return it otherwise. Only
if type assertion fails do we allocate new *big.Int.

Now, as we do not allocate every time, we had to change the API calls to
allocate a new result, so that we wouldn't mutate the passed in values.

At every API call utils.FromInterface method was called. This method always
allocates new *big.Int, which is very stressful for the runtime. Instead, we
first check if the variable is already *big.Int and return it otherwise. Only
if type assertion fails do we allocate new *big.Int.

Now, as we do not allocate every time, we had to change the API calls to
allocate a new result, so that we wouldnt mutate the passed in values.
@ivokub ivokub added the perf label Jun 23, 2022
@ivokub ivokub added this to the v0.8.0 milestone Jun 23, 2022
@ivokub ivokub requested a review from gbotrel June 23, 2022 22:52
@ivokub ivokub self-assigned this Jun 23, 2022
return b
func (e *engine) toBigInt(i1 frontend.Variable) *big.Int {
switch vv := i1.(type) {
case *big.Int:
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't we still do a mod reduce if it's a big int? may slow things down (or we could have a fast path with a comparaison first) --> you can have circuit inputs / constants at this stage, so it may trigger weird edge cases if we allow some values in the test engine to not be mod reduced?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried to rewrite the API methods such that toBigInt is only called for method inputs and I always recreate a new *big.Int for temp variables and result. The operations and results are always mod reduced.

I also tried to mod reduce the inputs in toBigInt method, but as I will be modifying inputs, I started having a few failing edge cases for the integration tests. Another approach would be to allocate a new *big.Int, set its value from the input and mod reduce it, but this goes against the goal of this PR which was to prevent unnecessary allocations.

@ivokub ivokub merged commit 3a7800d into develop Jul 6, 2022
@ivokub ivokub deleted the perf/test-engine branch July 6, 2022 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants