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

Integrate FidryCpuCoreCounter #8833

Merged
merged 4 commits into from Dec 10, 2022
Merged

Conversation

theofidry
Copy link
Contributor

@theofidry theofidry commented Dec 4, 2022

See theofidry/cpu-core-counter#11 for the motivations.

The code should be sensibly the same, the notable differences are:

  • a runtime exception NumberOfCpuCoreNotFound is thrown instead of LogicException (can easily be changed)
  • fixed the deprecated usage of hw.ncpu

I would also wait on 1.x before merging this if you're happy with it. Technically the current version is fine, I'm just looking for a round of feedback before publishing 1.0.

@orklah
Copy link
Collaborator

orklah commented Dec 4, 2022

Yeah, I'm pretty happy with that, I'd hate to have to tweak this somehow if something were to change on that front. Please ping us when 1.0 is released so we can merge this :)

Thanks for the comments on previous checks before. Shouldn't those checks be included in your library too?

@theofidry
Copy link
Contributor Author

Thanks for the comments on previous checks before. Shouldn't those checks be included in your library too?

Not really because this is really specific to Psalm, although I admit I don't know the why. For any of those items the library does work though and so would spawning several processes

@orklah
Copy link
Collaborator

orklah commented Dec 4, 2022

ok! Looking at the comments, I thought this was a common known issue with multi threading

@theofidry
Copy link
Contributor Author

Should be good; there is now a distinction possible between physical and logical cores. The default picked is logical (as PHP source does and as was done previously here).

If you fancy checking out what finder finds what, to find a bug or play around with the order or something, I can recommend those commands

@orklah orklah added the release:internal The PR will be included in 'Internal changes' section of the release notes label Dec 10, 2022
@orklah orklah merged commit ef02ded into vimeo:master Dec 10, 2022
@orklah
Copy link
Collaborator

orklah commented Dec 10, 2022

Thanks!

@theofidry theofidry deleted the feature/cpu-counter branch December 10, 2022 12:05
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

3 participants