Skip to content

Conversation

@hatemosphere
Copy link
Member

@hatemosphere hatemosphere commented Jun 14, 2025

What ❔

Optimize quotient evaluator with batch field inversions:

  • Collects all values to invert (4 per row) into a single batch
  • Performs one batch inversion operation instead of 4 individual inversions
  • Properly distributes inverted values back to their respective divisor computations

Why ❔

Tackling the TODO.
~5x average speedup for field inversion operations

Is this a breaking change?

  • Yes
  • No

Checklist

  • PR title corresponds to the body of PR (we generate changelog entries from PRs).
  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • Code has been formatted.

@0xVolosnikov 0xVolosnikov requested review from mm-zk and shamatar June 17, 2025 17:24
Copy link
Contributor

@0xVolosnikov 0xVolosnikov left a comment

Choose a reason for hiding this comment

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

Generally LGTM.

But please also get a review from @mm-zk or @shamatar since I'm not very deeply in the context here (e.g. I don't know if it affects peak RAM or smth).

@shamatar
Copy link
Member

I look at the code and have a hard time to understand what it did before and why this way. Divisors are in the form of (X^N - 1)/(X - 1), but we only divide by them, so batch inversions are not even needed in that form - (X^N - 1) is constant, and instead it's easy to compute just (X - 1)/(X^N - 1) and multiply by them later. I'll check properly when back from vacation

@hatemosphere
Copy link
Member Author

hatemosphere commented Jun 18, 2025

I look at the code and have a hard time to understand what it did before and why this way. Divisors are in the form of (X^N - 1)/(X - 1), but we only divide by them, so batch inversions are not even needed in that form - (X^N - 1) is constant, and instead it's easy to compute just (X - 1)/(X^N - 1) and multiply by them later. I'll check properly when back from vacation

can't really comment on that, since i was just trying to blindly follow up on simplest TODO i've found, to get the grasp of the code base, also having very crappy math background.

looks like:

  • for 0-1 divisors we are fine already with no inversion due to (x - omega^i)/(x^N - 1)
  • i've tried to implement inversion for 2-5. if i understand you correctly, the suggestion is to do (x^N - 1)/(x - omega^i) which still requires (to some degree expensive)division. possible better option - getting rid of inversions by completely changing the quotient evaluation logic, which is definitely beyond what i tried to do :D

ps. i still naively believe this change brings some perf improvements as is

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