Skip to content

Reduce memory consumption in FilesCollection #1219

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

Open
ckuran wants to merge 1 commit into
base: v2.x
Choose a base branch
from
Open

Reduce memory consumption in FilesCollection #1219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions spec/Locator/ChangedFilesSpec.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ function it_will_list_all_diffed_files(GitRepository $repository, Filesystem $fi
$result->shouldBeAnInstanceOf(FilesCollection::class);
$result[0]->getPathname()->shouldBe('file1.txt');
$result[1]->getPathname()->shouldBe('file2.txt');
$result->getIterator()->count()->shouldBe(2);
$result->shouldHaveCount(2);
}

function it_will_not_list_non_existing_files(GitRepository $repository, Filesystem $filesystem, Diff $diff, WorkingCopy $workingCopy)
Expand All @@ -74,7 +74,7 @@ function it_will_not_list_non_existing_files(GitRepository $repository, Filesyst

$result = $this->locateFromGitRepository();
$result->shouldBeAnInstanceOf(FilesCollection::class);
$result->getIterator()->count()->shouldBe(0);
$result->shouldHaveCount(0);
}

function it_will_list_all_diffed_files_from_raw_diff_input(GitRepository $repository, Filesystem $filesystem)
Expand All @@ -96,6 +96,6 @@ function it_will_list_all_diffed_files_from_raw_diff_input(GitRepository $reposi
$result = $this->locateFromRawDiffInput($rawDiff);
$result->shouldBeAnInstanceOf(FilesCollection::class);
$result[0]->getPathname()->shouldBe('file.txt');
$result->getIterator()->count()->shouldBe(1);
$result->shouldHaveCount(1);
}
}
2 changes: 1 addition & 1 deletion spec/Locator/ListedFilesSpec.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,6 @@ function it_will_list_all_listed_files()
$result->shouldBeAnInstanceOf(FilesCollection::class);
$result[0]->getPathname()->shouldBe('file1.txt');
$result[1]->getPathname()->shouldBe('file2.txt');
$result->getIterator()->count()->shouldBe(2);
$result->shouldHaveCount(2);
}
}
86 changes: 69 additions & 17 deletions src/Collection/FilesCollection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,12 +213,21 @@ public function filterByFileList(Traversable $fileList): FilesCollection

public function ensureFiles(self $files): FilesCollection
{
$newFiles = new self($this->toArray());
$newFiles = new self();
$knownPaths = [];

foreach (parent::getIterator() as $value) {
$newFiles->add($value);
$knownPaths[$value->getPathname()] = true;
}

foreach ($files as $file) {
if (!$newFiles->contains($file)) {
$newFiles->add($file);
$path = $file->getPathname();
if (isset($knownPaths[$path])) {
continue;
}
$newFiles->add($file);
$knownPaths[$path] = true;
}

return $newFiles;
Expand All @@ -234,14 +243,28 @@ public function ignoreSymlinks(): FilesCollection
/**
* SplFileInfo cannot be serialized. Therefor, we help PHP a bit.
* This stuff is used for running tasks in parallel.
*
* @psalm-suppress DocblockTypeContradiction
* @psalm-suppress RedundantConditionGivenDocblockType
*/
public function __serialize(): array
{
return $this->map(function (SplFileInfo $fileInfo): string {
return $fileInfo instanceof SymfonySplFileInfo
? $fileInfo->getRelativePathname()
: $fileInfo->getPathname();
})->toArray();
$paths = [];
foreach (parent::getIterator() as $value) {
if (\is_string($value)) {
$paths[] = $value;
continue;
}
if ($value instanceof SymfonySplFileInfo) {
$paths[] = $value->getRelativePathname();
continue;
}
if ($value instanceof SplFileInfo) {
$paths[] = $value->getPathname();
}
}

return $paths;
}

/**
Expand All @@ -250,25 +273,54 @@ public function __serialize(): array
*/
public function __unserialize(array $data): void
{
$files = $data;
$this->clear();
foreach ($files as $file) {
$this->add(new SymfonySplFileInfo($file, dirname($file), $file));
foreach ($data as $path) {
$this->add($path);
Copy link
Copy Markdown
Contributor

@veewee veewee May 22, 2026

Choose a reason for hiding this comment

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

The signature however is ArrayCollection<array-key, \SplFileInfo>.
Now it also contains strings making it result in broken public API contracts (see e.g; the getFirst() vs first() and optionally similar methods like last, current, ...)

I'm also wondering: now every time an iterator goes over it, it will be hydrated.
So if you have more tasks (or file filters), it will always increase memory.

This makes me wonder: what is the ACTUAL memory issue that we are trying to solve?
Can we find a better solution than keeping it as strings intermediate?

It also has been a while but: what benifit does SymfonySplFileInfo give over regular SplFileInfo here. Which one does add the overhead?

}
}

/**
* Help Psalm out a bit:
*
* @return \ArrayIterator<array-key, SplFileInfo>
* @return \Generator<array-key, SplFileInfo>
*/
public function getIterator(): \ArrayIterator
public function getIterator(): \Generator
{
return new \ArrayIterator($this->toArray());
foreach (parent::getIterator() as $key => $value) {
yield $key => $this->hydrateFileInfo($value);
}
}

public function toFileList(): string
{
return \implode(PHP_EOL, $this->toArray());
$lines = [];
foreach ($this as $file) {
$lines[] = $file->getPathname();
}

return \implode(PHP_EOL, $lines);
}

public function getFirst(): SplFileInfo|false
{
$value = parent::first();

return $value === false ? false : $this->hydrateFileInfo($value);
}

/**
* @param mixed $value
*/
private function hydrateFileInfo($value): SplFileInfo
{
if ($value instanceof SplFileInfo) {
return $value;
}

if (\is_string($value)) {
return new SymfonySplFileInfo($value, \dirname($value), $value);
}

throw new \LogicException(
'Unexpected element type in FilesCollection: ' . get_debug_type($value)
);
}
}
10 changes: 10 additions & 0 deletions src/Console/Command/Git/CommitMsgCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,16 @@ public function execute(InputInterface $input, OutputInterface $output): int

$results = $this->taskRunner->run($context);

if ($this->io->isVerbose()) {
$this->io->write([
PHP_EOL,
sprintf(
'<comment>Peak memory usage: %.1f MB</comment>',
(float) memory_get_peak_usage(true) / 1024.0 / 1024.0
)
]);
}

return $results->isFailed() ? self::EXIT_CODE_NOK : self::EXIT_CODE_OK;
}

Expand Down
10 changes: 10 additions & 0 deletions src/Console/Command/Git/PreCommitCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,16 @@ public function execute(InputInterface $input, OutputInterface $output): int

$results = $this->taskRunner->run($context);

if ($this->io->isVerbose()) {
$this->io->write([
PHP_EOL,
sprintf(
'<comment>Peak memory usage: %.1f MB</comment>',
(float) memory_get_peak_usage(true) / 1024.0 / 1024.0
)
]);
}

return $results->isFailed() ? self::EXIT_CODE_NOK : self::EXIT_CODE_OK;
}

Expand Down
10 changes: 10 additions & 0 deletions src/Console/Command/RunCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,16 @@ public function execute(InputInterface $input, OutputInterface $output): int

$results = $this->taskRunner->run($context);

if ($this->io->isVerbose()) {
$this->io->write([
PHP_EOL,
sprintf(
'<comment>Peak memory usage: %.1f MB</comment>',
(float) memory_get_peak_usage(true) / 1024.0 / 1024.0
)
]);
}

return $results->isFailed() ? self::EXIT_CODE_NOK : self::EXIT_CODE_OK;
}

Expand Down
8 changes: 6 additions & 2 deletions src/Task/Composer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,12 @@ public function run(ContextInterface $context): TaskResultInterface
return TaskResult::createFailed($this, $context, $this->formatter->format($process));
}

$composerFile = $files->first();
if ($config['no_local_repository'] && $composerFile && $this->hasLocalRepository($composerFile)) {
$composerFile = $files->getFirst();
if (false === $composerFile) {
return TaskResult::createFailed($this, $context, 'Could not find composer.json and composer.lock files.');
}

if ($config['no_local_repository'] && $this->hasLocalRepository($composerFile)) {
return TaskResult::createFailed($this, $context, 'You have at least one local repository declared.');
}

Expand Down
4 changes: 2 additions & 2 deletions test/Unit/Task/JsonLintTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public static function provideFailsOnStuff(): iterable
self::mockContext(RunContext::class, ['hello.json']),
function (array $options, ContextInterface $context) {
$this->assumeLinterConfig($options);
$this->linter->lint($context->getFiles()->first())->willThrow(new RuntimeException('nope'));
$this->linter->lint($context->getFiles()->getFirst())->willThrow(new RuntimeException('nope'));
},
'nope'
];
Expand All @@ -79,7 +79,7 @@ function (array $options, ContextInterface $context) {
self::mockContext(RunContext::class, ['hello.json']),
function (array $options, ContextInterface $context) {
$this->assumeLinterConfig($options);
$this->linter->lint($context->getFiles()->first())->willReturn(
$this->linter->lint($context->getFiles()->getFirst())->willReturn(
new LintErrorsCollection([
self::createLintError('hello.json'),
self::createLintError('hello.json'),
Expand Down
2 changes: 1 addition & 1 deletion test/Unit/Task/PhpParserTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ public static function provideFailsOnStuff(): iterable
self::mockContext(RunContext::class, ['hello.php']),
function (array $options, ContextInterface $context) {
$this->assumeParserConfig($options);
$this->parser->parse($context->getFiles()->first())->willReturn(new ParseErrorsCollection([
$this->parser->parse($context->getFiles()->getFirst())->willReturn(new ParseErrorsCollection([
self::createParseError('hello.php'),
self::createParseError('hello.php'),
]));
Expand Down
4 changes: 2 additions & 2 deletions test/Unit/Task/XmlLintTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ public static function provideFailsOnStuff(): iterable
self::mockContext(RunContext::class, ['hello.xml']),
function (array $options, ContextInterface $context) {
$this->assumeLinterConfig($options);
$this->linter->lint($context->getFiles()->first())->willThrow(new RuntimeException('nope'));
$this->linter->lint($context->getFiles()->getFirst())->willThrow(new RuntimeException('nope'));
},
'nope'
];
Expand All @@ -90,7 +90,7 @@ function (array $options, ContextInterface $context) {
self::mockContext(RunContext::class, ['hello.xml']),
function (array $options, ContextInterface $context) {
$this->assumeLinterConfig($options);
$this->linter->lint($context->getFiles()->first())->willReturn(
$this->linter->lint($context->getFiles()->getFirst())->willReturn(
new LintErrorsCollection([
self::createLintError('hello.xml'),
self::createLintError('hello.xml'),
Expand Down
4 changes: 2 additions & 2 deletions test/Unit/Task/YamlLintTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ public static function provideFailsOnStuff(): iterable
self::mockContext(RunContext::class, ['hello.yml']),
function (array $options, ContextInterface $context) {
$this->assumeLinterConfig($options);
$this->linter->lint($context->getFiles()->first())->willThrow(new RuntimeException('nope'));
$this->linter->lint($context->getFiles()->getFirst())->willThrow(new RuntimeException('nope'));
},
'nope'
];
Expand All @@ -90,7 +90,7 @@ function (array $options, ContextInterface $context) {
self::mockContext(RunContext::class, ['hello.yml']),
function (array $options, ContextInterface $context) {
$this->assumeLinterConfig($options);
$this->linter->lint($context->getFiles()->first())->willReturn(
$this->linter->lint($context->getFiles()->getFirst())->willReturn(
new LintErrorsCollection([
self::createLintError('hello.yml'),
self::createLintError('hello.yml'),
Expand Down
Loading