Allow config to define thread count

[M1ke]

For large projects many systems will not handle multithreading correctly. For example if run in a container, the underlying machine may report 32 threads, but the container has access to 2. Or in large projects where there's a memory limit (e.g. our codebase takes 7GB) running with more than 1 thread, even if possible, consumes all available memory.

Whilst threads can be set on the command line, this isn't always possible. My specific example is the way that PhpStorm runs Psalm, but I imagine there are other tools which may run Psalm off a default config or with their own flags, where a user cannot configure what flags are set. Generally it's worth most flags being settable in a config file anyway, so that behaviour can be locked down - for example in our project for local developers, we have added a set of bash scripts to run Psalm with various flags, and all of them force threads=1.

This PR adds "threads" as a config option. If set, and if lower than the detected threads, it will be applied. If unset or higher than detected threads, existing behaviour is applied. Thus this cannot disrupt existing mechanisms of setting threads to 1.

I've been unable to run tests locally due to a weird bug with composer that's failing to unzip the symyony/filesystem plugin, so I will watch the CI build and make adjustments if required.

[orklah]

Cool! This is a great addition!

if lower than the detected threads, it will be applied

I'm not a fan of that behaviour. I'd prefer if we could use the config instead of the detection and then override that with special cases for --debug or other hardcoded rules

[M1ke]

I'd prefer if we could use the config instead of the detection

My concern about that is that the config can then produce unexpected results. I.e. I run on my CI infrastructure usually which has 16 cores, so I configure it to use 14 of them. However one time I try to run locally and I'm honestly not sure what happens if you try and run 14 threads on a machine that has 4 cores. So then you're having to either run in debug mode (limited to 1 thread when maybe you'd prefer to use 2) or having to edit the config file (and remembering not to commit your change)

I guess we could run it in order of:

• Things that force to 1 (debug, CI)
• Command line
• Config file
• Detection

However I think we'd still want detection to be used if the config is set, to make sure it doesn't use too many threads. Which is basically back to the implementation I've done here.

[M1ke]

I can see that there's one failing job but not sure how to resolve; I can see the labels section on the PR sidebar but not clear how I add one. Is that something only maintainers can do?

What do you think about how we should handle the order of precedence here?

[orklah]

As far as I know, the only risk of running on too many threads is loosing performances.

Either because there is no sense trying to run two threads on the same core or because each thread need some memory and that could sometimes use all the memory the machine has (and then it's either killed by the OS or stopped by php's memory limit)

On the other hand, we know that the detection in CI is limited so it's only a question of time before we have a user that want to fix the detection in the other way...

The last job can only fixed by us, don't worry about it ;)

[M1ke]

Cool, so if I modify to this order:

* Things that force to 1 (debug, CI)
* Command line
* Config file
* Detection

Would you be happy to merge?

[M1ke]

Actually I realise that exact order would be a breaking change because currently the command line takes precedence, so I'll do:

• Command line
• Things that force to 1 (debug, CI)
• Config file
• Detection

[AndrolGenhald]

Actually I realise that exact order would be a breaking change because currently the command line takes precedence, so I'll do:

• Command line
• Things that force to 1 (debug, CI)
• Config file
• Detection

Just to give my 2 cents, this is the order I would expect, and I wouldn't expect a command line specifying 50 threads to be limited to 8 because that's what Psalm detects. I can imagine a case where Psalm's runtime is bounded by some sort of latency more than by CPU time. Perhaps someone has their code on a network filesystem with unfortunately high latency, it's possible running Psalm with more threads than cores would improve runtime depending on the file access pattern.

I wouldn't expect CI to force 1 thread, but if that's the way it works now I guess I'm fine leaving it.

[M1ke]

Right, functionality updated per the suggestions, and all green. Need anything else from me or lgtm?

[orklah]

Thanks! Congrats on your first PR!

[M1ke]

Thanks, glad to be of service and as you've seen I've already got my second (slightly more ambitious) one in! Psalm is great!

…

On Thu, 20 Jan 2022, 6:39 pm orklah, ***@***.***> wrote: Thanks! Congrats on your first PR! — Reply to this email directly, view it on GitHub <#7442 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJLLC6CVL6TKTHKUPDFUYDUXBJFNANCNFSM5MMJJ2YQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you authored the thread.Message ID: ***@***.***>

[M1ke]

@orklah is there a way to know when this makes it into a release? I can see that 4.19 covers commits before and after the date this was merged, but it doesn't seem to have been included. Or have I got my merge target wrong for this?

[orklah]

@M1ke your PR targeted the master branch, who is our development branch for the next major version of Psalm (so Psalm 5).

We should have released a first alpha a few days ago but we're kinda late on that. The real version will be released in a few weeks

Your PR didn't contain BC break so it could theoretically have targeted 4.x...

[M1ke]

@orklah ok thanks; if I were to submit the same commits as a PR onto 4.x would you be happy to merge?

[orklah]

@M1ke I have no objection, but I also have very little experience with git :p

@weirdan what is the standard procedure in this case?

[M1ke]

I can attempt a rebase - depends what's changed in the underlying code but I don't think I've touched much that seems to be modified heavily in 5.x

If I can't rebase, I'll write the same content onto 4.x and submit that as a PR. Once merged I can open a PR from 4.x to master which will resolve potential merge conflicts.

[weirdan]

what is the standard procedure in this case?

Rebase and another PR should do the trick, I think.