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.

Sign up for GitHub

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

Jump to bottom

chore(data-structure): rework exceptions #325

Merged
merged 1 commit into from
Dec 24, 2021
Merged

chore(data-structure): rework exceptions #325

merged 1 commit into from
Dec 24, 2021

Conversation

azjezz
Copy link
Owner

@azjezz azjezz commented Dec 23, 2021 •
edited
Loading

  1. throw underflow exception instead of invariant violation ( ref rework exceptions #198 )
  2. optimize data structures by using builtin functions instead of PSL ( while messier, it's faster, and is not exposed in the public API )

@azjezz azjezz added Priority: High After critical issues are fixed, these should be dealt with before any further issues. Status: Accepted It's clear what the subject of the issue is about, and what the resolution should be. Status: Review Needed The issue has a PR attached to it which needs to be reviewed. Type: Enhancement Most issues will probably ask for additions or changes. Type: BC Break A change that will result in a backward compatibility break in the public API. labels Dec 23, 2021
@azjezz azjezz added this to the 2.0.0 milestone Dec 23, 2021
@azjezz azjezz requested a review from veewee December 23, 2021 18:26
@azjezz azjezz self-assigned this Dec 23, 2021
@coveralls
Copy link

coveralls commented Dec 23, 2021 •
edited
Loading

Pull Request Test Coverage Report for Build 1616906397

  • 25 of 25 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.0002%) to 99.881%
Totals Coverage Status
Change from base Build 1616895328: -0.0002%
Covered Lines: 3344
Relevant Lines: 3348
💛 - Coveralls

@azjezz azjezz force-pushed the chore/ds branch 3 times, most recently from fe4e701 to 187fb49 Compare December 23, 2021 19:03
jrmajor
jrmajor reviewed Dec 23, 2021
View reviewed changes
src/Psl/DataStructure/PriorityQueue.php Outdated
/**
* Retrieve the list of nodes with the priority `$priority`.
* shit the first node out.
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean shift? :)

Copy link
Owner Author

Choose a reason for hiding this comment

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

LMAO

@azjezz
Copy link
Owner Author

azjezz commented Dec 23, 2021

@veewee okay with this change? ( trying to make exceptions make sense a bit, invariant violation everywhere seemed a bit too generic, and doesn't allow for multiple catch blocks depending on the issue )

@veewee
Copy link
Collaborator

veewee commented Dec 23, 2021 •
edited
Loading

Sure, makes sense!
At first sight it's ok, but gonna go through it tomorrow on my computer with a fresh mind.

@azjezz
chore(data-structure): rework exceptions
d346e90 
Signed-off-by: azjezz <azjezz@protonmail.com>
@veewee
Copy link
Collaborator

veewee commented Dec 24, 2021

Looks good!
Any idea about the speed improvements by using array functions instead of psl internally?

@azjezz
Copy link
Owner Author

azjezz commented Dec 24, 2021 •
edited
Loading

should be 2 to 3 times as fast ( no just because of that change, i did some other changes as well ).

also, Psl\Iter\Iterator in 2.0.x is 12 times faster than the one in 1.9.x ;)

@azjezz azjezz merged commit b803594 into 2.0.x Dec 24, 2021
@azjezz azjezz deleted the chore/ds branch December 24, 2021 12:12
@veewee
Copy link
Collaborator

veewee commented Dec 24, 2021

Nice!

@azjezz
Copy link
Owner Author

azjezz commented Dec 24, 2021

Oh, Collection is also 3 times faster ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrmajor jrmajor jrmajor left review comments

@veewee veewee Awaiting requested review from veewee

Assignees

@azjezz azjezz

Labels
Priority: High After critical issues are fixed, these should be dealt with before any further issues. Status: Accepted It's clear what the subject of the issue is about, and what the resolution should be. Status: Review Needed The issue has a PR attached to it which needs to be reviewed. Type: BC Break A change that will result in a backward compatibility break in the public API. Type: Enhancement Most issues will probably ask for additions or changes.
Projects
None yet
Milestone
2.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@azjezz @coveralls @veewee @jrmajor