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

chore: try add small optimizations to divide_polynomialcoeff #3722

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

kevaundray
Copy link
Contributor

This adds three optimizations:

  • Replace .insert(0,x) with .append(x) and then a .reverse. Inserting at the first index on each loop causes every item in the list to be shifted down by one at each loop iteration.

  • return [x % BLS_MODULUS for x in o] to return o . The values of o should already be reduced since they are coming from the div function.

  • (a - b + modulus) becomes (a + (modulus - b)) - This mainly a nit, as the order of operations is not entirely clear.

@kevaundray
Copy link
Contributor Author

This shaves off around 2 minutes from the 7594 tests (looked at other PRs to see how long they took compared to this)

Copy link
Contributor

@asn-d6 asn-d6 left a comment

Choose a reason for hiding this comment

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

This looks correct to me!

I'm approving but not merging, in case we want to discuss this further. Also curious how much each of the three (two?) optimizations contribute to the overall performance improvement: "returning o" is a clear improvement in performance and readability, whereas the append/reverse is slightly non-trivial (still fine though).

@kevaundray
Copy link
Contributor Author

Good points -- I'm happy to leave this open until there's consensus

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.

None yet

2 participants