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

BYO signing sorter impl. #447

Merged
merged 34 commits into from
Dec 19, 2024
Merged

Conversation

ximon18
Copy link
Member

@ximon18 ximon18 commented Nov 25, 2024

Allows consumers to e.g. use Rayon for faster multi-threaded sorting instead of the default slow single-threaded sorting, without placing a hard-dependency on Rayon in the domain crate.

Also changes the NSEC3 logic to no longer insert into a self-sorting collection while collecting NSEC3s, instead it now pushes to an unsorted vec then sorts that before iterating, as post-sorting is much faster.

…eparate `FormatWriter` and (b) that only uses tabs between record fields and not between rdata values.
…for faster multi-threaded sorting instead of the default slow single-threaded sorting.

Also, don't insert into a self-sorting collection while collecting NSEC3s, instead push to an unsorted vec then sort before iterating, as post-sorting is much faster.
@ximon18 ximon18 requested a review from a team November 25, 2024 13:27
Copy link
Contributor

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

I like the idea, but I'm not sure whether this is the right API? I would expect something like records.sort_with(some_function_to_sort) -> SortedRecords, where the sorter is not part of the SortedRecords type, but this introduces a bunch of additional trait bounds (and we already have so many). Another option to reduce the trait bounds is to implement extend on the Sorter, so you make a Sorter and then let that insert into the SortedRecords.

@ximon18
Copy link
Member Author

ximon18 commented Nov 25, 2024

I like the idea, but I'm not sure whether this is the right API? I would expect something like records.sort_with(some_function_to_sort) -> SortedRecords, where the sorter is not part of the SortedRecords type, but this introduces a bunch of additional trait bounds (and we already have so many). Another option to reduce the trait bounds is to implement extend on the Sorter, so you make a Sorter and then let that insert into the SortedRecords.

The reason I didn't do that was I found it to be a foot gun, it was too easy to not use the sort in all of the places it should be used, I felt that it's a fundamental characteristic of the instance which sorting algorithm/impl it uses, not just something for one method call.

@ximon18
Copy link
Member Author

ximon18 commented Nov 25, 2024

One issue for example is that nsec3() and sign() do sorting themselves so there's no opportunity to do the sorting from outside, except to pass the sorter in at multiple points which would be both unwieldy, and feels strange to potentially let you use different sort impls at different stages of using the same object.

Maybe the problem is that the fns should be broken down into pieces, or not be methods on a collection or in some other way that a different design entirely than SortedRecords is called for.

I don't know at this point, I just wanted one place to define the sort impl to use consistently with the SortedRecords type once created.

@ximon18 ximon18 marked this pull request as ready for review December 19, 2024 08:24
@ximon18 ximon18 merged commit 27f5bd2 into multiple-key-signing Dec 19, 2024
5 of 17 checks passed
@ximon18 ximon18 deleted the byo-signing-sorter-impl branch December 19, 2024 08:25
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