Skip to content
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

Add property typehints in Internal directory #8897

Merged
merged 9 commits into from Dec 15, 2022

Conversation

jack-worman
Copy link
Contributor

Added more property typehints but just for in the Internal directory to ensure BC.

I then used php-cs-fixer to clean up redundant phpdocs.

.php-cs-fixer.dist.php Outdated Show resolved Hide resolved
src/Psalm/Internal/Analyzer/CanAlias.php Show resolved Hide resolved
@weirdan
Copy link
Collaborator

weirdan commented Dec 14, 2022

@jack-worman would you mind running benchmarks against this PR and the master branch? It looks like it could have more performance impact than your previous PR.

@jack-worman
Copy link
Contributor Author

jack-worman commented Dec 14, 2022

@jack-worman would you mind running benchmarks against this PR and the master branch? It looks like it could have more performance impact than your previous PR.

OLD (5.2.0):
256.11s user 20.99s system 342% cpu 1:20.90 total
251.93s user 19.63s system 351% cpu 1:17.33 total
252.73s user 19.57s system 347% cpu 1:18.33 total
254.36s user 19.10s system 350% cpu 1:17.92 total
252.01s user 19.30s system 352% cpu 1:16.94 total
247.46s user 18.03s system 340% cpu 1:17.97 total

NEW (this branch):
246.72s user 19.21s system 346% cpu 1:16.84 total
243.94s user 18.63s system 354% cpu 1:14.13 total
244.22s user 18.06s system 357% cpu 1:13.39 total
249.78s user 19.64s system 346% cpu 1:17.82 total
260.73s user 21.50s system 336% cpu 1:23.89 total
255.39s user 19.22s system 354% cpu 1:17.42 total

Using eyeball statistics, I would say there is no performance difference.
I ran this on my jobs fairly large codebase.

@jack-worman jack-worman force-pushed the Add_some_more_property_typehints branch from 33ecb1a to 76a7c7d Compare December 15, 2022 02:43
@weirdan weirdan added the release:internal The PR will be included in 'Internal changes' section of the release notes label Dec 15, 2022
Copy link
Collaborator

@weirdan weirdan left a comment

Choose a reason for hiding this comment

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

LGTM.

@orklah, @danog any objections?

@orklah
Copy link
Collaborator

orklah commented Dec 15, 2022

Should we ignore backward compatibiliy checks here?

@jack-worman
Copy link
Contributor Author

Should we ignore backward compatibiliy checks here?

Those BC breaks are because those files were missing their @internal annotation, even though they were in the Psalm\Internal namespace.

@danog
Copy link
Collaborator

danog commented Dec 15, 2022

If there are no performance impacts (and according to your tests performance improved even, were you using opcache? If yes, the typehints might've triggered some optimizations), LGTM!

@orklah orklah merged commit e69f88a into vimeo:master Dec 15, 2022
@orklah
Copy link
Collaborator

orklah commented Dec 15, 2022

Thanks!

@jack-worman jack-worman deleted the Add_some_more_property_typehints branch December 15, 2022 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:internal The PR will be included in 'Internal changes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants