Skip to content

Lazy initialization of AnalyserResult::$errors #2400

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

Merged
merged 1 commit into from
Jul 1, 2023

Conversation

takaram
Copy link
Contributor

@takaram takaram commented May 13, 2023

AnalyserResult::$errors array is sorted in the class' constructor, but it doesn't always need to be sorted.
This PR makes the property be initialized when getErrors is called.

In AnalyseApplication::analyse we may get two AnalyserResult instances. One of them is just an intermediate result, so its error array doesn't have to be sorted.
Sorting an array with usort may take a few seconds if the array has thousands of elements.

@takaram takaram changed the base branch from 1.11.x to 1.10.x May 13, 2023 17:28
@phpstan-bot
Copy link
Collaborator

You've opened the pull request against the latest branch 1.11.x. If your code is relevant on 1.10.x and you want it to be released sooner, please rebase your pull request and change its target to 1.10.x.

@ondrejmirtes
Copy link
Member

Can you please share a use case where AnalyserResult is created but errors aren't read? Thank you.

@mad-briller
Copy link
Contributor

@ondrejmirtes i think this isn't in relation to them being created but not read, but more to avoid the cost of sorting twice

on line 80 of AnalyseApplication there is this:

$intermediateAnalyserResult = $this->runAnalyser(

which produces an AnalyserResult and as such has sorted the errors once,
but then later on on line 94 there is this:

$intermediateAnalyserResult = new AnalyserResult(
	array_merge($intermediateAnalyserResult->getErrors(), $stubErrors),

which will re-sort the errors, including any additional errors produced in stubs

this change makes it so that the errors are only sorted once

@ondrejmirtes
Copy link
Member

@mad-briller Are you sure? There's $intermediateAnalyserResult->getErrors() call in the code you posted.

@mad-briller
Copy link
Contributor

yeah, the getErrors call in the code i posted returns already-sorted errors, which will then be sorted again in the AnalyserResult constructor

@ondrejmirtes
Copy link
Member

But after this PR they will get sorted twice anyway, right?

@mad-briller
Copy link
Contributor

mad-briller commented Jul 1, 2023

the pr actually changes the call to getError in the second instance to a getUnorderedErrors, the code samples i used were from before the change had happened to explain what it was targeting

			$intermediateAnalyserResult = new AnalyserResult(
					array_merge($intermediateAnalyserResult->getUnorderedErrors(), $stubErrors),
					```

@ondrejmirtes
Copy link
Member

I get it now 😊

@ondrejmirtes ondrejmirtes merged commit 8329c64 into phpstan:1.10.x Jul 1, 2023
@ondrejmirtes
Copy link
Member

Thank you.

@takaram
Copy link
Contributor Author

takaram commented Jul 1, 2023

@mad-briller
Thank you so much for answering the question! I didn't have enough time to answer😅

@takaram takaram deleted the lazy_error_sorting branch July 1, 2023 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants