Skip to content

Remove slow assertReadableFile() check#1345

Merged
Ocramius merged 2 commits into
Roave:6.10.xfrom
staabm:perf2
May 15, 2023
Merged

Remove slow assertReadableFile() check#1345
Ocramius merged 2 commits into
Roave:6.10.xfrom
staabm:perf2

Conversation

@staabm
Copy link
Copy Markdown
Contributor

@staabm staabm commented May 15, 2023

this line showed up in my profilling and I realized most call-sites already do file-IO on the given filename before creating a LocatedSource, e.g.

grafik

if ($filename !== null) {
assert($filename !== '');

FileChecker::assertReadableFile($filename);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Worth using realpath here?

Copy link
Copy Markdown
Contributor Author

@staabm staabm May 15, 2023

Choose a reason for hiding this comment

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

I did not find a single place where we pass in filename, but did not already do IO on the path, like e.g. file_get_contents.
I think this check here is useless.

we could hide it behand a if(zend-assertions-enabled) switch or similar, if you prefer.

I don't get why realpath() would be different then what we have right now?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This API is public, so yah, some invariant checking would be expected.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, never mind, it's @internal

I'll 🚢 as-is, thanks @staabm!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you 🙏

if ($filename !== null) {
assert($filename !== '');

FileChecker::assertReadableFile($filename);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, never mind, it's @internal

I'll 🚢 as-is, thanks @staabm!

@Ocramius Ocramius changed the title Remove slow assertReadableFile() check Remove slow assertReadableFile() check May 15, 2023
@Ocramius Ocramius self-assigned this May 15, 2023
@Ocramius Ocramius added this to the 6.10.0 milestone May 15, 2023
@Ocramius Ocramius merged commit af7f78f into Roave:6.10.x May 15, 2023
@staabm staabm deleted the perf2 branch May 15, 2023 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants