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
--
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
