From 87124dabed11bd7be911c0afe543872880c2c3c2 Mon Sep 17 00:00:00 2001 From: Pol Dellaiera Date: Thu, 17 Dec 2020 19:03:24 +0100 Subject: [PATCH] refactor: Rewrite iterators and optimize things here and there. --- .../Iterator/ArrayCacheIteratorSpec.php | 50 ++++++++++ ...ratorSpec.php => PsrCacheIteratorSpec.php} | 6 +- .../Iterator/RandomIteratorSpec.php | 48 +++++----- src/Collection.php | 10 +- src/Iterator/ArrayCacheIterator.php | 93 +++++++++++++++++++ src/Iterator/ClosureIterator.php | 53 +++++++++++ src/Iterator/IterableIterator.php | 23 ++--- ...CacheIterator.php => PsrCacheIterator.php} | 37 ++++++-- src/Iterator/RandomIterator.php | 71 ++++++-------- src/Iterator/ResourceIterator.php | 2 - src/Iterator/StringIterator.php | 2 - src/Operation/Cache.php | 4 +- 12 files changed, 300 insertions(+), 99 deletions(-) create mode 100644 spec/loophp/collection/Iterator/ArrayCacheIteratorSpec.php rename spec/loophp/collection/Iterator/{CacheIteratorSpec.php => PsrCacheIteratorSpec.php} (92%) create mode 100644 src/Iterator/ArrayCacheIterator.php create mode 100644 src/Iterator/ClosureIterator.php rename src/Iterator/{CacheIterator.php => PsrCacheIterator.php} (56%) diff --git a/spec/loophp/collection/Iterator/ArrayCacheIteratorSpec.php b/spec/loophp/collection/Iterator/ArrayCacheIteratorSpec.php new file mode 100644 index 000000000..01b662a2d --- /dev/null +++ b/spec/loophp/collection/Iterator/ArrayCacheIteratorSpec.php @@ -0,0 +1,50 @@ +beConstructedWith($generator()); + + $this + ->valid() + ->shouldReturn(true); + + if (5 !== iterator_count($this->getWrappedObject())) { + throw new Exception('The count is invalid.'); + } + + $this + ->shouldIterateAs( + range('a', 'e') + ); + } + + public function it_is_initializable() + { + $this->beConstructedWith(new ArrayIterator([])); + + $this->shouldHaveType(ArrayCacheIterator::class); + } +} diff --git a/spec/loophp/collection/Iterator/CacheIteratorSpec.php b/spec/loophp/collection/Iterator/PsrCacheIteratorSpec.php similarity index 92% rename from spec/loophp/collection/Iterator/CacheIteratorSpec.php rename to spec/loophp/collection/Iterator/PsrCacheIteratorSpec.php index e74c08e38..36eb17298 100644 --- a/spec/loophp/collection/Iterator/CacheIteratorSpec.php +++ b/spec/loophp/collection/Iterator/PsrCacheIteratorSpec.php @@ -6,12 +6,12 @@ use ArrayIterator; use Iterator; -use loophp\collection\Iterator\CacheIterator; +use loophp\collection\Iterator\PsrCacheIterator; use PhpSpec\ObjectBehavior; use Psr\Cache\CacheItemPoolInterface; use Symfony\Component\Cache\CacheItem; -class CacheIteratorSpec extends ObjectBehavior +class PsrCacheIteratorSpec extends ObjectBehavior { public function it_can_cache_data(CacheItemPoolInterface $cache) { @@ -88,6 +88,6 @@ public function it_can_get_the_inner_iterator(Iterator $iterator, CacheItemPoolI public function it_is_initializable(Iterator $iterator, CacheItemPoolInterface $cache) { $this->beConstructedWith($iterator, $cache); - $this->shouldHaveType(CacheIterator::class); + $this->shouldHaveType(PsrCacheIterator::class); } } diff --git a/spec/loophp/collection/Iterator/RandomIteratorSpec.php b/spec/loophp/collection/Iterator/RandomIteratorSpec.php index 8eb66d0ee..016eb692a 100644 --- a/spec/loophp/collection/Iterator/RandomIteratorSpec.php +++ b/spec/loophp/collection/Iterator/RandomIteratorSpec.php @@ -23,30 +23,30 @@ public function it_can_build_an_iterator_with_a_random_seed() $expected = [ 2 => 'c', + 20 => 'u', + 8 => 'i', + 3 => 'd', + 7 => 'h', + 9 => 'j', + 0 => 'a', + 21 => 'v', + 12 => 'm', + 15 => 'p', + 13 => 'n', 4 => 'e', + 19 => 't', + 10 => 'k', 22 => 'w', - 21 => 'v', - 20 => 'u', - 14 => 'o', + 11 => 'l', + 1 => 'b', 5 => 'f', + 18 => 's', + 23 => 'x', 17 => 'r', - 25 => 'z', 24 => 'y', - 10 => 'k', - 13 => 'n', - 23 => 'x', - 15 => 'p', - 8 => 'i', - 11 => 'l', - 0 => 'a', - 7 => 'h', - 19 => 't', - 9 => 'j', - 3 => 'd', 16 => 'q', - 18 => 's', - 1 => 'b', - 12 => 'm', + 25 => 'z', + 14 => 'o', 6 => 'g', ]; @@ -54,8 +54,8 @@ public function it_can_build_an_iterator_with_a_random_seed() throw new Exception('Iterator is not equal to the expected array.'); } - $iterator1 = new RandomIterator($input, $seed); - $iterator2 = new RandomIterator($input, $seed + $seed); + $iterator1 = new RandomIterator(new ArrayIterator(range('a', 'z')), $seed); + $iterator2 = new RandomIterator(new ArrayIterator(range('a', 'z')), $seed + $seed); if (iterator_to_array($iterator1) === iterator_to_array($iterator2)) { throw new Exception('Iterator1 is equal to Iterator2'); @@ -67,7 +67,7 @@ public function it_can_build_an_iterator_without_a_random_seed() $input = new ArrayIterator(range('a', 'z')); $this->beConstructedWith($input); - $iterator1 = new RandomIterator($input); + $iterator1 = new RandomIterator(new ArrayIterator(range('a', 'z'))); if (iterator_to_array($iterator1) === iterator_to_array($this->getWrappedObject())) { throw new Exception('Iterator1 is equal to Iterator2'); @@ -85,6 +85,12 @@ public function it_can_rewind() $this->beConstructedWith($iterator, 1); + $this + ->valid() + ->shouldReturn(false); + + $this->rewind(); + $this ->valid() ->shouldReturn(true); diff --git a/src/Collection.php b/src/Collection.php index 12edf98d2..2df734cb7 100644 --- a/src/Collection.php +++ b/src/Collection.php @@ -10,6 +10,7 @@ use IteratorIterator; use loophp\collection\Contract\Collection as CollectionInterface; use loophp\collection\Contract\Operation; +use loophp\collection\Iterator\ClosureIterator; use loophp\collection\Iterator\IterableIterator; use loophp\collection\Iterator\ResourceIterator; use loophp\collection\Iterator\StringIterator; @@ -425,10 +426,7 @@ public function get($key, $default = null): CollectionInterface public function getIterator(): Iterator { - $iterator = new IteratorIterator(($this->source)(...$this->parameters)); - $iterator->rewind(); - - return $iterator; + return new ClosureIterator($this->source, ...$this->parameters); } public function group(): CollectionInterface @@ -657,9 +655,7 @@ public function scanRight1(callable $callback): CollectionInterface public function shuffle(?int $seed = null): CollectionInterface { - if (null === $seed) { - $seed = random_int(PHP_INT_MIN, PHP_INT_MAX); - } + $seed ??= random_int(PHP_INT_MIN, PHP_INT_MAX); return new self(Shuffle::of()($seed), $this->getIterator()); } diff --git a/src/Iterator/ArrayCacheIterator.php b/src/Iterator/ArrayCacheIterator.php new file mode 100644 index 000000000..b12f57770 --- /dev/null +++ b/src/Iterator/ArrayCacheIterator.php @@ -0,0 +1,93 @@ + + */ +final class ArrayCacheIterator extends ProxyIterator +{ + /** + * @psalm-var array + */ + private array $cache = []; + + private int $key = 0; + + /** + * @psalm-param Iterator $iterator + */ + public function __construct(Iterator $iterator) + { + $this->iterator = $iterator; + } + + /** + * @psalm-return T + */ + public function current() + { + /** @psalm-var array{TKey, T} $data */ + $data = $this->getTupleFromCache($this->key); + + return $data[1]; + } + + /** + * @psalm-return TKey + */ + public function key() + { + /** @psalm-var array{TKey, T} $data */ + $data = $this->getTupleFromCache($this->key); + + return $data[0]; + } + + public function next(): void + { + // This is mostly for iterator_count(). + $this->getTupleFromCache($this->key++); + + parent::next(); + } + + public function rewind(): void + { + // No call to parent::rewind() because we do not know if the inner + // iterator can be rewinded or not. + $this->key = 0; + } + + public function valid(): bool + { + if (parent::valid()) { + return true; + } + + return array_key_exists($this->key, $this->cache); + } + + /** + * @psalm-return array{0: TKey, 1: T} + */ + private function getTupleFromCache(int $key): array + { + return $this->cache[$key] ??= [ + parent::key(), + parent::current(), + ]; + } +} diff --git a/src/Iterator/ClosureIterator.php b/src/Iterator/ClosureIterator.php new file mode 100644 index 000000000..7740ee1dc --- /dev/null +++ b/src/Iterator/ClosureIterator.php @@ -0,0 +1,53 @@ + + */ +final class ClosureIterator extends ProxyIterator +{ + /** + * @var array + * @psalm-var list + */ + private array $arguments; + + /** + * @var callable + * @psalm-var callable(mixed ...):Generator + */ + private $callable; + + /** + * @param mixed ...$arguments + * @psalm-param mixed ...$arguments + * @psalm-param callable(mixed ...):Generator $callable + */ + public function __construct(callable $callable, ...$arguments) + { + $this->callable = $callable; + $this->arguments = $arguments; + $this->iterator = $this->getGenerator(); + } + + public function rewind(): void + { + $this->iterator = $this->getGenerator(); + } + + /** + * @psalm-return Generator + */ + private function getGenerator(): Generator + { + return yield from ($this->callable)(...$this->arguments); + } +} diff --git a/src/Iterator/IterableIterator.php b/src/Iterator/IterableIterator.php index 06f5bd58d..2e302cce0 100644 --- a/src/Iterator/IterableIterator.php +++ b/src/Iterator/IterableIterator.php @@ -4,10 +4,7 @@ namespace loophp\collection\Iterator; -use ArrayIterator; -use IteratorIterator; - -use function is_array; +use Generator; /** * @psalm-template TKey @@ -23,12 +20,16 @@ final class IterableIterator extends ProxyIterator */ public function __construct(iterable $iterable) { - if (is_array($iterable)) { - $iterable = new ArrayIterator($iterable); - } - - $this->iterator = new IteratorIterator($iterable); - - $this->rewind(); + $this->iterator = new ClosureIterator( + /** + * @psalm-param iterable $iterable + */ + static function (iterable $iterable): Generator { + foreach ($iterable as $key => $value) { + yield $key => $value; + } + }, + $iterable + ); } } diff --git a/src/Iterator/CacheIterator.php b/src/Iterator/PsrCacheIterator.php similarity index 56% rename from src/Iterator/CacheIterator.php rename to src/Iterator/PsrCacheIterator.php index 2da80f45c..5327560c8 100644 --- a/src/Iterator/CacheIterator.php +++ b/src/Iterator/PsrCacheIterator.php @@ -9,16 +9,20 @@ use Psr\Cache\CacheItemPoolInterface; /** + * @internal + * * @psalm-template TKey * @psalm-template TKey of array-key * @psalm-template T * * @extends ProxyIterator */ -final class CacheIterator extends ProxyIterator +final class PsrCacheIterator extends ProxyIterator { private CacheItemPoolInterface $cache; + private int $key = 0; + /** * @psalm-param Iterator $iterator */ @@ -34,7 +38,7 @@ public function __construct(Iterator $iterator, CacheItemPoolInterface $cache) public function current() { /** @psalm-var array{TKey, T} $data */ - $data = $this->getItemOrSave((string) $this->iterator->key())->get(); + $data = $this->getTupleFromCache($this->key)->get(); return $data[1]; } @@ -45,26 +49,39 @@ public function current() public function key() { /** @psalm-var array{TKey, T} $data */ - $data = $this->getItemOrSave((string) $this->iterator->key())->get(); + $data = $this->getTupleFromCache($this->key)->get(); return $data[0]; } - public function valid(): bool + public function next(): void + { + // This is mostly for iterator_count(). + $this->getTupleFromCache($this->key++); + + parent::next(); + } + + public function rewind(): void { - $key = (string) $this->iterator->key(); + // No call to parent::rewind() because we do not know if the inner + // iterator can be rewinded or not. + $this->key = 0; + } - return '' !== $key && ($this->iterator->valid() || $this->cache->hasItem($key)); + public function valid(): bool + { + return parent::valid() || $this->cache->hasItem((string) $this->key); } - private function getItemOrSave(string $key): CacheItemInterface + private function getTupleFromCache(int $key): CacheItemInterface { - $item = $this->cache->getItem($key); + $item = $this->cache->getItem((string) $key); if (false === $item->isHit()) { $item->set([ - $this->iterator->key(), - $this->iterator->current(), + parent::key(), + parent::current(), ]); $this->cache->save($item); diff --git a/src/Iterator/RandomIterator.php b/src/Iterator/RandomIterator.php index 3fdf8a01e..988c88d4f 100644 --- a/src/Iterator/RandomIterator.php +++ b/src/Iterator/RandomIterator.php @@ -4,11 +4,7 @@ namespace loophp\collection\Iterator; -use ArrayIterator; use Iterator; -use OuterIterator; - -use function array_slice; use const PHP_INT_MAX; use const PHP_INT_MIN; @@ -18,28 +14,23 @@ * @psalm-template TKey of array-key * @psalm-template T of string * - * @implements Iterator + * @extends ProxyIterator */ -final class RandomIterator implements Iterator, OuterIterator +final class RandomIterator extends ProxyIterator { /** * @var array */ - private array $indexes; - - /** - * @psalm-var Iterator - */ - private Iterator $iterator; + private array $indexes = []; - private int $key; + private ?int $key = 0; private int $seed; /** - * @psalm-var ArrayIterator + * @psalm-var Iterator */ - private ArrayIterator $wrappedIterator; + private Iterator $wrappedIterator; /** * @psalm-param Iterator $iterator @@ -48,42 +39,38 @@ public function __construct(Iterator $iterator, ?int $seed = null) { $this->iterator = $iterator; $this->seed = $seed ?? random_int(PHP_INT_MIN, PHP_INT_MAX); - $this->wrappedIterator = $this->buildArrayIterator($iterator); - $this->indexes = array_keys($this->wrappedIterator->getArrayCopy()); - $this->key = current($this->customArrayRand($this->indexes, 1, $this->seed)); + $this->wrappedIterator = new ArrayCacheIterator($iterator); } public function current() { - $value = $this->wrappedIterator[$this->key]; + $keyValueTuple = $this->getNextItemAtKey($this->key); - return $value[1]; - } - - public function getInnerIterator() - { - return $this->iterator; + return $keyValueTuple[1]; } public function key() { - $value = $this->wrappedIterator[$this->key]; + $keyValueTuple = $this->getNextItemAtKey($this->key); - return $value[0]; + return $keyValueTuple[0]; } public function next(): void { unset($this->indexes[$this->key]); - if ($this->valid()) { - $this->key = current($this->customArrayRand($this->indexes, 1, $this->seed)); - } + $this->key = key($this->indexes); } public function rewind(): void { - $this->indexes = array_keys($this->wrappedIterator->getArrayCopy()); + // @todo: Try to get rid of iterator_count(). + $this->indexes = $this->predictableRandomArray( + range(0, iterator_count($this->wrappedIterator) - 1), + $this->seed + ); + $this->key = 0; } public function valid(): bool @@ -92,28 +79,30 @@ public function valid(): bool } /** - * @psalm-param Iterator $iterator + * We do not cache the values in here. + * It's already done in the ArrayCacheIterator. * - * @psalm-return ArrayIterator + * @psalm-return array{0: TKey, 1: T} */ - private function buildArrayIterator(Iterator $iterator): ArrayIterator + private function getNextItemAtKey(int $key): array { - /** @psalm-var ArrayIterator $arrayIterator */ - $arrayIterator = new ArrayIterator(); + $i = 0; + + $this->wrappedIterator->rewind(); - foreach ($iterator as $key => $value) { - $arrayIterator->append([$key, $value]); + while ($this->indexes[$key] !== $i++) { + $this->wrappedIterator->next(); } - return $arrayIterator; + return [$this->wrappedIterator->key(), $this->wrappedIterator->current()]; } - private function customArrayRand(array $array, int $num, int $seed): array + private function predictableRandomArray(array $array, int $seed): array { mt_srand($seed); shuffle($array); mt_srand(); - return array_slice($array, 0, $num); + return $array; } } diff --git a/src/Iterator/ResourceIterator.php b/src/Iterator/ResourceIterator.php index 88edef2ea..fbad850d4 100644 --- a/src/Iterator/ResourceIterator.php +++ b/src/Iterator/ResourceIterator.php @@ -39,7 +39,5 @@ static function ($resource): Generator { }; $this->iterator = new IteratorIterator($callback($resource)); - - $this->rewind(); } } diff --git a/src/Iterator/StringIterator.php b/src/Iterator/StringIterator.php index 56185be4e..0171ab2da 100644 --- a/src/Iterator/StringIterator.php +++ b/src/Iterator/StringIterator.php @@ -40,7 +40,5 @@ static function (string $input, string $delimiter): Generator { }; $this->iterator = new IteratorIterator($callback($data, $delimiter)); - - $this->rewind(); } } diff --git a/src/Operation/Cache.php b/src/Operation/Cache.php index 408034971..ea6dec692 100644 --- a/src/Operation/Cache.php +++ b/src/Operation/Cache.php @@ -6,7 +6,7 @@ use Closure; use Iterator; -use loophp\collection\Iterator\CacheIterator; +use loophp\collection\Iterator\PsrCacheIterator; use Psr\Cache\CacheItemPoolInterface; /** @@ -33,6 +33,6 @@ public function __invoke(): Closure * * @psalm-return Iterator */ - static fn (Iterator $iterator): Iterator => new CacheIterator($iterator, $cache); + static fn (Iterator $iterator): Iterator => new PsrCacheIterator($iterator, $cache); } }