Skip to content

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.

3 participants