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.

Sign up for GitHub

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

Jump to bottom

perf: avoid copying on group operations #383

Merged
merged 21 commits into from
Apr 16, 2024
Merged

perf: avoid copying on group operations #383

merged 21 commits into from
Apr 16, 2024

Conversation

chokobole
Copy link
Contributor

@chokobole chokobole commented Apr 6, 2024 •
edited by ashjeong

Description

This PR avoids copying on semigroup operations, which are Sub(), Div(), Negative() and Inverse().

@chokobole chokobole force-pushed the perf/avoid-copying-on-group-operations branch 10 times, most recently from 5174f25 to 6b8f82d Compare April 12, 2024 04:46
@chokobole chokobole marked this pull request as ready for review April 12, 2024 04:52
dongchangYoo
dongchangYoo approved these changes Apr 12, 2024
View reviewed changes
Copy link
Contributor

@dongchangYoo dongchangYoo left a comment

Choose a reason for hiding this comment

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

LGTM

@chokobole chokobole force-pushed the perf/avoid-copying-on-group-operations branch 2 times, most recently from 7ccddfe to 23cc757 Compare April 13, 2024 08:21
ashjeong
ashjeong requested changes Apr 15, 2024
View reviewed changes
Copy link
Contributor

@ashjeong ashjeong left a comment •
edited

Choose a reason for hiding this comment

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

57ed88e - I see differences in checks of l_evaluations and r_evaluations size. Is this intended?
Multilinear_extension_ops - CHECK_EQ
Univariate_evaluations_ops - no check
Univariate_polynomial_ops - resized to max size of the two

db8934e and 05f6452
Appear as though they should belong together

tachyon/math/base/groups.h Outdated Show resolved Hide resolved
tachyon/math/base/groups.h Show resolved Hide resolved
tachyon/math/polynomials/univariate/univariate_polynomial.h Show resolved Hide resolved
tachyon/math/polynomials/univariate/univariate_polynomial_ops.h Show resolved Hide resolved
tachyon/math/base/groups.h Show resolved Hide resolved
tachyon/math/finite_fields/generator/prime_field_generator/prime_field_x86.h.tpl Outdated Show resolved Hide resolved
tachyon/math/polynomials/univariate/univariate_evaluations_ops.h Outdated Show resolved Hide resolved
tachyon/math/polynomials/univariate/univariate_evaluations_ops.h Show resolved Hide resolved
tachyon/math/polynomials/univariate/univariate_sparse_coefficients.h Outdated Show resolved Hide resolved
tachyon/math/polynomials/univariate/univariate_sparse_coefficients.h Outdated Show resolved Hide resolved
@chokobole chokobole force-pushed the perf/avoid-copying-on-group-operations branch from f0e3772 to 9ad6752 Compare April 16, 2024 10:07
Insun35
Insun35 approved these changes Apr 16, 2024
View reviewed changes
Copy link
Contributor

@Insun35 Insun35 left a comment

Choose a reason for hiding this comment

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

LGTM

TomTaehoonKim
TomTaehoonKim approved these changes Apr 16, 2024
View reviewed changes
Copy link
Contributor

@TomTaehoonKim TomTaehoonKim left a comment

Choose a reason for hiding this comment

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

LGTM

@chokobole chokobole force-pushed the perf/avoid-copying-on-group-operations branch from 9ad6752 to 9c38390 Compare April 16, 2024 11:55
@ashjeong
Copy link
Contributor

please fix 69d8eb8
fix(math): fix fast return cases for the polynomial operations
->
fix(math): fix fast return case for a polynomial operation

@chokobole
refac(math): add missing changes from #379
03e2b84
@chokobole
perf(math): add missing change from #379
85d7db6
@chokobole
refac(math): replace DivInPlace() with MulInPlace() and Inverse()
3db82c0
@chokobole
refac(math): remove redundant DivInPlace()
18c315b
@chokobole
refac(c/math): use assignment operator instead of XXXInPlace()
063875b
@chokobole
refac: remove Div() and DivInPlace()
298a799
@chokobole
perf(math): avoid copying in polynomial division
2f10974
@chokobole
perf(math): avoid copying in polynomial modulo operation
0bac619
@chokobole
perf(math): avoid copying in subtraction
0a16654
@chokobole
perf: avoid copying in inverse
f1e6b55
@chokobole
perf: avoid copying in negative
8153ba5
@chokobole
perf(math): avoid copying in cyclotomic square
c7856e4
@chokobole
perf(math): avoid copying in cyclotomic inverse
45e4d7d
@chokobole
perf(math): avoid copying in cyclotomic pow
716cbaf
@chokobole
perf(math): return reference if possible
d733af8
@chokobole
fix(math): fix fast return case for a polynomial operation
5941c8f
@chokobole
fix(math): ensure to clear all the terms if sparse poly is multiplied…
6c20646 
… by zero
@chokobole
fix(math): fix folding sparse univariate polynomial to remove zero field
c6bc0f4
@chokobole
fix(math): add missing RemoveHighDegreeZeros()
02e10be
@chokobole
refac(math): remove redundant base::AlwaysFalse<G>()
bb37a03 
`operator/=()` and `operator-=()` won't be used if proper methods are
not implemented in the derived classes. That's why this statement is
redundant.
@chokobole
feat(math): add missing size check to the evaluation operations
cd6748d
@chokobole chokobole force-pushed the perf/avoid-copying-on-group-operations branch from 9c38390 to cd6748d Compare April 16, 2024 13:13
ashjeong
ashjeong approved these changes Apr 16, 2024
View reviewed changes
Copy link
Contributor

@ashjeong ashjeong left a comment

Choose a reason for hiding this comment

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

LGTM

dongchangYoo
dongchangYoo approved these changes Apr 16, 2024
View reviewed changes
Copy link
Contributor

@dongchangYoo dongchangYoo left a comment

Choose a reason for hiding this comment

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

LGTM

@chokobole chokobole merged commit 38a2ff1 into main Apr 16, 2024
3 checks passed
@chokobole chokobole deleted the perf/avoid-copying-on-group-operations branch April 16, 2024 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dongchangYoo dongchangYoo dongchangYoo approved these changes

@Insun35 Insun35 Insun35 approved these changes

@TomTaehoonKim TomTaehoonKim TomTaehoonKim approved these changes

@ashjeong ashjeong ashjeong approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@chokobole @ashjeong @dongchangYoo @Insun35 @TomTaehoonKim