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

Use a parallel sort to order txs while speculating #2425

Merged

Conversation

ljedrz
Copy link
Collaborator

@ljedrz ljedrz commented Apr 5, 2024

A small follow-up to https://github.com/AleoHQ/snarkVM/pull/2421 and the replacement of https://github.com/AleoHQ/snarkVM/pull/2424. The changes are:

  • a cfg_sort_unstable_by macro for parallel sorting using a closure is introduced (note: the unstable part only means that equal elements may be reordered, and transaction IDs are never equal)
  • the aforementioned macro is used to sort transactions when speculating
  • the parallel iteration for the collection is removed, as it is slower than single-threaded iteration (since Transaction::id is basically free)
  • instead of enumerating and collecting indices, IndexSet::get_index_of is used

Signed-off-by: ljedrz <ljedrz@gmail.com>
@ljedrz ljedrz requested a review from howardwu April 5, 2024 13:03
@howardwu
Copy link
Contributor

howardwu commented Apr 6, 2024

Is the assumption that sort_unstable is safe because the IDs are supposed to be unique? If so, this should be documented in the code itself for future readers to know.

Signed-off-by: ljedrz <ljedrz@gmail.com>
@ljedrz
Copy link
Collaborator Author

ljedrz commented Apr 7, 2024

The unstable sort is safe, it only may reorder equal elements.

However, I realized that due to lookups, caching the key would be more meaningful here; I added a macro for sorting by cached key and adjusted this PR to use it.
I kept the macro for unstable sorting, as it may be useful in the future, for applications where they key extraction function is cheap.

@howardwu howardwu merged commit 51c3d27 into AleoNet:mainnet-staging Apr 12, 2024
80 checks passed
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.

4 participants