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: non-deterministic array sort #4279

Merged
merged 7 commits into from
Feb 7, 2024
Merged

chore: non-deterministic array sort #4279

merged 7 commits into from
Feb 7, 2024

Conversation

guipublic
Copy link
Contributor

@guipublic guipublic commented Feb 6, 2024

Description

Problem*

Resolves #4171
We have an intrinsic sort function to do sorting on numeric arrays. It allows field elements but fail with them.
At the same time we have a more generic sort_via method with works with custom ordering function but it has bad performance.

Summary*

I have rationalised the sorting functions for arrays, we now have only one which works on any type and support custom ordering functions.
The new sort function benefits from dynamic arrays and does not need sorting networks, so I was able to remove a lot of deprecated code. See below for the impact on performance.

The issue 4171 is resolved by specifying the Ord trait for array elements.

Additional Context

So we have 3 sort functions:
the new one 'ultra_sort'
the pre-ultra plonk 'array_sort'
the customisable one, 'sort_via'
With this PR, we only keep the new one 'ultra_sort'. Here how it compares with the others:

Sorting an array of 10 elements:

  • Ultra sort: 99 Acir gates, 202 UltraPlonk gates
  • Array sort: 235 Acir gates, 279 UltraPlonk gates
  • Sort_via: 1733 Acir gates, 1867 UltraPlonk gates

Sorting an array of 100 elements:

  • Ultra sort: 999 Acir gates, 1597 UltraPlonk gates
  • Array sort: 6674 Acir gates, 6673 UltraPlonk gates
  • Sort_via: 1524713 Acir gates, 1522136 UltraPlonk gates

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [Exceptional Case] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

Copy link
Contributor

github-actions bot commented Feb 6, 2024

Changes to circuit sizes

Generated at commit: 15e34e73cdd95c265b92d5908a8dfef68a3dde19, compared to commit: fd150521a480c04ff64f84e3c1a2faf1e8394516

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
array_sort -3 ✅ -8.82% +14 ❌ +17.07%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
array_sort 31 (-3) -8.82% 96 (+14) +17.07%

@guipublic
Copy link
Contributor Author

This PR also resolves #4261

Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

👍

@jfecher jfecher added this pull request to the merge queue Feb 7, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 7, 2024
@jfecher jfecher added this pull request to the merge queue Feb 7, 2024
Merged via the queue into master with commit 2ffef26 Feb 7, 2024
33 checks passed
@jfecher jfecher deleted the gd/ultra_sort branch February 7, 2024 18:21
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.

Update sort functions in stdlib, and their documentation
2 participants