-
-
Notifications
You must be signed in to change notification settings - Fork 35
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
Performance improvements using iterator aggregates #233
Conversation
a55efd4
to
4b9d0e7
Compare
6fffeaf
to
6f89a4e
Compare
Looks good at a first glance, let me know when it's ready for review and I'll go through it more in-depth 👍 |
626c023
to
0b06339
Compare
I think it's done. This is something I should have done a long time ago. |
0b06339
to
192734d
Compare
192734d
to
68ffdee
Compare
Pull Request is not mergeable
68ffdee
to
d623bc4
Compare
Pull Request is not mergeable
d623bc4
to
1fe15b5
Compare
1fe15b5
to
5c431b1
Compare
3346313
to
e72f268
Compare
Pull request was converted to draft
e72f268
to
bee8a2a
Compare
bee8a2a
to
1be166e
Compare
1be166e
to
e2b5dcc
Compare
d775074
to
36f1062
Compare
25ed276
to
83405dc
Compare
83405dc
to
a07f7e2
Compare
private $source; | ||
|
||
/** | ||
* @psalm-external-mutation-free |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is removing this intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think we should even get rid of them everywhere. psalm doesn't handle them correctly anyway. I really wanted to avoid updating the baselines too much.
} | ||
|
||
/** | ||
* @pure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed some of these @pure
annotations are being removed, but not everywhere. I think just in src/Collection.php
?
How come this is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as the previous one. We are not the only ones... https://github.com/marcosh/lamphpda/blob/master/adr/2022-01-25-pure-callable.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as the previous one. We are not the only ones... https://github.com/marcosh/lamphpda/blob/master/adr/2022-01-25-pure-callable.md
I get this one, but in this project we didn't go that far to use pure-callable
types 😬 .
I think the errors you saw in this project might be related to the iterators project not having things marked as @pure
.
In any case it might be too much of a hassle to maintain this kind of stuff in PHP, in which case I think it's fine to remove them but we can do it for the whole codebase. You can do it in a separate PR though
This PR:
IterableIterator
withIterableIteratorAggregate
(4 occurences)Iterator
is needed while now aniterable
is enough.yield from
- align everythingClosureIterator
and replace it withClosureIteratorAggregate
Based on the benchmarks I did in
loophp/iterators
, it seems to be definitely, and it make sense.IteratorAggregates
are usually faster because the state of theIterator
is not built in userland but directly handled by PHP internaly.@AlexandruGG WDYT of this? Are you ok with this?