Configure max processes by free ones

[yguedidi]

inspired by https://tomasvotruba.com/blog/hidden-productivity-costs-of-parallel-run

[yguedidi]

@TomasVotruba @samsonasik any feedback on this? :)

[TomasVotruba]

Could you extend test as well?

[yguedidi]

@TomasVotruba could you point me to the test to extend? didn't manage to find it. Or if it's a new kind of test, what would be the best approach?

[TomasVotruba]

Basically extend or add ScheduleFactoryTest, that includes various inputs and expected results.

[yguedidi]

@TomasVotruba not sure I get you 😕
The new logic is in ApplicationFileProcessor::runParallel(), not in ScheduleFactory.
I don't see neither how a ScheduleFactoryTest will test this logic, neither how to test ApplicationFileProcessor properly

[TomasVotruba]

It will need a test coverage. That might require a bit refactoring, so testing is easier :)

[yguedidi]

Good that you're open to some refactoring! Will come with a solution when I'll have time ;)

[yguedidi]

@TomasVotruba done! :)

[staabm]

since the parameter $maxNumberOfProcess now has in-depth semantics I think we need a phpdoc explaining the different use-cases one can do with it

[yguedidi]

very good idea! phpdoc added, ping @TomasVotruba

[TomasVotruba]

Thanks 👍

I don't understand this part:

and a negative one to preserve that number of CPU cores.

Feels like magic. The value here should be only positive integer.

[yguedidi]

@TomasVotruba thanks for the feedback! what would be your ideal API to configure preserving CPU cores?

To me it feels logical, I read "$maxNumberOfProcess = -1" as "minus one CPU core from all available CPU cores", or in a more lengthy way "perserve one CPU core amoung all available CPU cores"
Any idea how to make this clearer?
It would be too much and confusing to add a new parameter for this preserve configuration IMO.

Goal is to provide a persistent configuration among a dev team, where devs don't have the same number of CPU cores on their laptop.

[yguedidi]

@TomasVotruba @samsonasik anything else needed to have this merged?

[yguedidi]

@TomasVotruba @samsonasik friendly reminder on this, it looks ready. I don't want to look too pushy sorry 😅

[samsonasik]

I will let @TomasVotruba decide on this :)

[TomasVotruba]

Thanks for the update. I get the idea and I like it.
I still feel the configuration of "max" having -1 would be really confusing and is not untuitive to use.

Like having max speed of - 20 k/h. Could be slow down? Or going backwords? Or reverse in the instant speed?

Saying that, I'm closing this one untill we find a better way.

[yguedidi]

@TomasVotruba the use of negative is inspired by PHP itself, see substr length parameter

It's the best compromise I managed to find between being BC compliant (existing config behavior will not change), relatively understandable, and bringing the feature to life inspired by your blog post.

If you have any better idea to configure it please tell me

[yguedidi]

@TomasVotruba maybe the issue is on the "max" wording? what if the parameter become just $numberOfProcess?

• 0 as default to use all available ones
• fixed positive to use that number when possible
• fixed negative to preserve that number from availabe ones when possible

What do you think?

[staabm]

what about adding a new method for the use-case?

[yguedidi]

@staabm to me it would be one more user facing option to maintain, maybe confusing users, as it's something really linked to using parallel.

side note: it would have been possible with something like in #3602 :)

[yguedidi]

@TomasVotruba before I rework this, please give me a configuration API you will accept. seems you liked the proposal of @staabm about using a new method, but what would be its name? parallelPreserveCpuCore(int)?

Again, to me it's more complex for end users to have to configure something related to parallelism in a separate method.
Also, it will mean having 2 different places ralated to the same thing: the parameter in parallel() and the new method. This makes things more complex to understand.
Finally, to we want to expose such a new configuration method and maintain it over time as a dedicated one?

@TomasVotruba any feedback on above hypothesis #3615 (comment) that the confusion may come from the current "max" naming? Maybe removing it from the name will at least reduce the confusion?

Thank you for your patience