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

Major rework of Rejection system #895

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

sidju
Copy link

@sidju sidju commented Sep 10, 2021

Split out the inline modules into their own files.
(reduce length of individual files, more predictable file structure)
Restructure from linked tree into vector.
(increase memory use, but probably reduce allocation latencies)
Add in a Fatal case into Rejections Reasons enum.
(to be converted into a response and returned immediately when occured)

David Olofsson and others added 2 commits September 10, 2021 12:39
Split out the inline modules into their own files.
(reduce length of individual files, more predictable file structure)
Restructure from linked tree into vector.
(increase memory use, but probably reduce allocation latencies)
Add in a Fatal case into Rejections Reasons enum.
(to be converted into a response and returned immediately when occured)
@seanmonstar
Copy link
Owner

Thanks for the PR! Some of this we can easily merge, and other parts I think need more discussion.

Split out the inline modules into their own files.

Seems like a good call!

Restructure from linked tree into vector. (increase memory use, but probably reduce allocation latencies)

Is this a measured improvement? I assume the common part of rejections is combining them, and the less common is part is searching the tree repeatedly, that's why I optimized for that. I also note that this change increases the size of Rejection, since it now has an enum with a variant of Vec, that is 3 usizes big.

Add in a Fatal case into Rejections Reasons enum. (to be converted into a response and returned immediately when occured)

We've talked a little bit about doing this, but it's not immediately clear that this is the right way forward. I do agree that there's probably something wrong with the current rejection design, since it's such a frequent cause of confusion.

@sidju
Copy link
Author

sidju commented Sep 10, 2021 via email

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.

2 participants