[cmake-developers] Ninja pools

[Peter Kümmel]

On 08.11.2013 17:22, Brad King wrote:
> On 11/05/2013 02:15 PM, Peter Kümmel wrote:
>> I merged a proposal to next which adds support for Ninja's "pool":
>>
>>     http://martine.github.io/ninja/manual.html#ref_pool
>
> This will be a really nice feature to support.
>

Thanks for the detailed comments. Below some remarks.

>> It adds three new properties, POOLS, LINK_POOL, COMPILE_POOL:
>>
>>     http://www.cmake.org/cmake/help/git-next/manual/cmake-properties.7.html
>>
>> With a "pool" it is possible to limit the number of concurrent
>> processes of a specific rule.
>
> Without context the name "POOL" may not have clear meaning to
> a reader not already familiar with Ninja's feature.  I think
> a name like JOB_POOL would be clearer.

Even JOB_POOL leaves the question what a "pool" is.
I think it would be better to completely drop "pool", and to use
something more obvious like PARALLEL_JOBS

>
> The documentation of these properties all note that they are
> only for Ninja.  While it is possible a future generator will
> also support them, it is unclear whether the semantics will be
> exactly the same.  Therefore I prefer to use a NINJA_ prefix
> on the property names for now:
>
>   NINJA_JOB_POOLS
>   NINJA_JOB_POOL_LINK
>   NINJA_JOB_POOL_COMPILE
>

I would prefer to create a semantic which also fits for other generators
and not to use the NINJA prefix. How could the limiting be very different
for other generators? It always reduces the number of parallel processes.

Maybe a property for just disabling parallel execution at all is enough.
I couldn't imagine use cases where the fine-grained pool control
of ninja is needed. Such a simplifies interface should also be
compatible with other generators, and would be very strict, as requested.

Peter



> Also look at cmTarget::SetMakefile for calls to SetPropertyDefault
> to see how to add variables like
>
>   CMAKE_NINJA_JOB_POOL_LINK
>   CMAKE_NINJA_JOB_POOL_COMPILE
>
> that set a default for the target properties at their creation.
> This will make it easy for projects to put all targets into the
> pools.  We should also consider a way to allow users to define
> pools with cache entries when the projects don't.
>
>> Current patch adds only the essentials, but maybe there are more
>> comfortable ways to use pools.
>
> The patch also appears to try adding a POOL option to the
> add_custom_command command but does not provide documentation
> except in the commit message.  In the final version of this
> topic that goes to master, I'd prefer not to have any docs or
> examples in the commit messages, just in the actual docs.
> (Again, I think the option should be named NINJA_JOB_POOL.)
>
> Should we have a
>
>   NINJA_JOB_POOL_CUSTOM
>
> property that is used for custom commands within a target
> that do not explicitly set a job pool?  It would of course
> come with a supporting CMAKE_NINJA_JOB_POOL_CUSTOM variable
> to initialize the property.  This would allow projects to
> quickly add all their build rules to pools with just a few
> lines of code.
>
> Also please add error messages when the NINJA_JOB_POOLS
> value does not match the expected format, or when one of
> the other rules names a pool not on the global list.  The
> add_custom_command option processing:
>
> +         case doing_pool:
> +           pool = copy;
> +           break;
>
> should switch back to doing = doing_nothing so that a second
> value after the keyword is rejected with an error message.
> (Some of the other keyword options should be doing this but
> can't now for compatibility.)
>
> The interface should be as strict as possible when first created.
> It can always be relaxed in the future, but would require a
> policy to make it more strict.  Please add RunCMake test cases
> that cover these error messages.
>
> Thanks,
> -Brad
> --
>
> Powered by www.kitware.com
>
> Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html
>
> Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ
>
> Follow this link to subscribe/unsubscribe:
> http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
>