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

Add helper method to inline mul_by_const immediately under constraints optimization goal #339

Closed
wants to merge 6 commits into from

Conversation

ValarDragon
Copy link
Member

@ValarDragon ValarDragon commented Jan 5, 2021

Description

This adds a method to constraint system that gadgets such as FpVar would call to do multiplication by a constant. It handles inlining immediately when under the Constraints Optimization goal, and doesn't create additional symbolic variables when the variable is Zero. This could cause speed losses to constraint-optimized circuits when doing two mul by consts in a row, but I think such a case isn't that likely.

This is intended to be used here: https://github.com/arkworks-rs/r1cs-std/blob/master/src/fields/fp/mod.rs#L192


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (master)
  • Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests - its hard to write unit tests in this repo
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

@weikengchen
Copy link
Member

Corrected one small typo deffering -> deferring. (note: if you are using a Mac, you might be a victim of the bufferfly keyboard like me https://support.apple.com/keyboard-service-program-for-mac-notebooks)

Others look good.

@Pratyush
Copy link
Member

Pratyush commented Jan 5, 2021

Do we need to make a new API for this? As in, inside new_lc, we can detect if the incoming linear combination consists of just a single term, and can inline it directly if we're in the Constraints optimization goal.

(This might even be helpful under any optimization goal, when the variable in the single term is not SymbolicLc).

@ValarDragon
Copy link
Member Author

ValarDragon commented Jan 5, 2021

That works, though it is unfortunate that we'd be doing wasted heap allocations then, versus producing the right lc. (One extra heap allocation for basically every operation) We don't really have empirical verification that thats a bottleneck though.

@Pratyush
Copy link
Member

Pratyush commented Jan 5, 2021

If we make LinearCombination to be a SmallVec of size 1 then we don't have to pay that overhead at all =)

@ValarDragon
Copy link
Member Author

True! I'll close this in favor of going with that approach

@ValarDragon ValarDragon closed this Jan 5, 2021
@ValarDragon
Copy link
Member Author

ValarDragon commented Jan 5, 2021

Actually, I'm realizing now that this actually increases memory overhead in the current design, since we're still storing each intermediate LC, and now those LC's are longer. (as would the new_lc approach) This type of approach would be useful if there were a drop-semantics style approach of removing old lc's from the lc_map.

@ValarDragon ValarDragon deleted the reduce_call_graph_const_depth branch January 5, 2021 19:28
@Pratyush
Copy link
Member

Pratyush commented Jan 5, 2021

I think there are two approaches to handle this:

  • Either we have linear combinations in the variable directly, or in the lc_map, but not in both. This way, if we inline LCs, then when the variable goes out of scope, the memory for the LC is freed immediately.

  • We hold a Rc<RefCell<LcMap>> or something inside a SymbolicLc, and when the SymbolicLc gets dropped, we look inside the reference to LcMap and do some garbage collection there. Not sure how expensive this would be, though.

@ValarDragon
Copy link
Member Author

We had the same ideas =) #cref #336 (comment)

Either we have linear combinations in the variable directly, or in the lc_map, but not in both. This way, if we inline LCs, then when the variable goes out of scope, the memory for the LC is freed immediately.

We still need LC's getting saved to the lc_map at some point though, in order to do density saving optimizations. Doing that at constraint generation causes some awkward mutability problems though.

@Pratyush
Copy link
Member

Pratyush commented Jan 5, 2021

Let's consolidate in #336

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.

3 participants