-
Notifications
You must be signed in to change notification settings - Fork 160
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
Feat/poly #216
Conversation
) | ||
|
||
// Memory management for polynomials | ||
// Copied verbatim from gkr repo |
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.
No longer true
@@ -121,3 +147,75 @@ func (p *Polynomial) Equal(p1 Polynomial) bool { | |||
|
|||
return true | |||
} | |||
|
|||
func signedBigInt(v *fr.Element) big.Int { |
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.
I've seen the Text
method does something similar to this. Should we extract that logic into a ToSignedBigInt
that Text
also calls?
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.
mmhh for the Text one, it actually did give me issues in the past; I think the Text / String should never output "negative" formating. And we should add a "prettyString" or something like that which does.
Current way it's done in Text doesn't allocate a big int I believe ?
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.
It does!
https://github.com/ConsenSys/gnark-crypto/blob/master/ecc/bn254/fp/element.go#L824
Also, in this case using a pool makes sense because big.Int.SetBytes
does sometimes reuse previously allocated memory correct?
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.
yes correct . We should by the way probably mutualize the big int pool declared in fr/ fp/ on each curves. Will create a ticket 👍
|
||
func (p Polynomial) Text(base int) string { | ||
//TODO: Okay to use bavard in non-code-generation context? | ||
builder := bavard.StringBuilderPool.Get().(*strings.Builder) |
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.
@gbotrel Is it actually useful to pool string builders? I read somewhere that when a builder is done, the buffer is handed to the generated string and another buffer would be allocated for the next use. If that's the case, it makes no sense to pool these.
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.
probably not useful, and fine to do like the go std string builder 👍
@Tabaie I don't understand what does |
In the GKR project they do a lot of extrapolations on small domains of the form |
The multilinear ones are denoted by their values on the hypercube. This Lagrange thing is for univariate functions. |
Ah ok. And why the FFT isn't used for this ? Are the degrees in play too small ? |
Yes the maximum size is 12. I think that's too small for FFT to pay off. What do you think? |
oh ok yes in that case it's better to do it by hands. |
Polynomial module from gkr project. If most things look good I'll clean up into a proper PR.