diff mbox series

[2/3] ci: do not use #processors+1 jobs, #processors is enough

Message ID 20210518084139.97957-3-pbonzini@redhat.com (mailing list archive)
State New, archived
Headers show
Series Small CI improvements | expand

Commit Message

Paolo Bonzini May 18, 2021, 8:41 a.m. UTC
I could not reconstruct the origin of the $(($(nproc) + 1)) idiom,
but I suspect it was there only to have a sensible result when nproc
or getconf do not exist.  This can be achieved also with an "||".

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 .gitlab-ci.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Philippe Mathieu-Daudé May 18, 2021, 8:56 a.m. UTC | #1
On 5/18/21 10:41 AM, Paolo Bonzini wrote:
> I could not reconstruct the origin of the $(($(nproc) + 1)) idiom,

I guess it is the historical way make would aggressively use
the most compute power it could? Then later this bad habit impact
was reduced by the -l option to keep make under some system load
value.

> but I suspect it was there only to have a sensible result when nproc
> or getconf do not exist.  This can be achieved also with an "||".
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  .gitlab-ci.yml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

BTW more candidates:

.gitlab-ci.d/edk2.yml:49: - export JOBS=$(($(getconf _NPROCESSORS_ONLN)
+ 1))
.gitlab-ci.d/opensbi.yml:52: - export JOBS=$(($(getconf
_NPROCESSORS_ONLN) + 1))
.travis.yml:93:  - export JOBS=3

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Thomas Huth May 18, 2021, 10:49 a.m. UTC | #2
On 18/05/2021 10.41, Paolo Bonzini wrote:
> I could not reconstruct the origin of the $(($(nproc) + 1)) idiom,
> but I suspect it was there only to have a sensible result when nproc
> or getconf do not exist.  This can be achieved also with an "||".
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   .gitlab-ci.yml | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index 4bd1a91aa8..3f0d86cf0a 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -17,7 +17,7 @@ include:
>     stage: build
>     image: $CI_REGISTRY_IMAGE/qemu/$IMAGE:latest
>     before_script:
> -    - JOBS=$(expr $(nproc) + 1)
> +    - JOBS=$(nproc || echo 1)

The basic idea of the "+ 1" was to make sure that there is always a thread 
that runs on a CPU while maybe another one is waiting for I/O to complete. 
This is suggested by various sites on the web, e.g.:

 
https://unix.stackexchange.com/questions/519092/what-is-the-logic-of-using-nproc-1-in-make-command

So not sure whether this patch here make sense ... I'd rather drop it.

  Thomas
Paolo Bonzini May 18, 2021, 12:30 p.m. UTC | #3
On 18/05/21 12:49, Thomas Huth wrote:
>>
>> -    - JOBS=$(expr $(nproc) + 1)
>> +    - JOBS=$(nproc || echo 1)
> 
> The basic idea of the "+ 1" was to make sure that there is always a 
> thread that runs on a CPU while maybe another one is waiting for I/O to 
> complete.

Ah, I see.  It doesn't make much sense for "make check" jobs however, 
which is where I wanted to get with the next patch.

I'm not sure it's even true anymore with current build machines (which 
tend to have a large buffer cache for headers) and optimizing compilers 
that compilation is I/O bound, so I'll time the two and see if there is 
an actual difference.

Paolo

  This is suggested by various sites on the web, e.g.:
> 
> https://unix.stackexchange.com/questions/519092/what-is-the-logic-of-using-nproc-1-in-make-command 
> 
> So not sure whether this patch here make sense ... I'd rather drop it.
Daniel P. Berrangé May 18, 2021, 12:43 p.m. UTC | #4
On Tue, May 18, 2021 at 02:30:04PM +0200, Paolo Bonzini wrote:
> On 18/05/21 12:49, Thomas Huth wrote:
> > > 
> > > -    - JOBS=$(expr $(nproc) + 1)
> > > +    - JOBS=$(nproc || echo 1)
> > 
> > The basic idea of the "+ 1" was to make sure that there is always a
> > thread that runs on a CPU while maybe another one is waiting for I/O to
> > complete.
> 
> Ah, I see.  It doesn't make much sense for "make check" jobs however, which
> is where I wanted to get with the next patch.
> 
> I'm not sure it's even true anymore with current build machines (which tend
> to have a large buffer cache for headers) and optimizing compilers that
> compilation is I/O bound, so I'll time the two and see if there is an actual
> difference.

I'd be surprised if you can measure any statistically reliable difference
at all wrt public CI. I've tried measuring CI performance for small changes
and found it impossible in short time frames, as the deviation between runs
is way too large. GitLab CI speeds tend to slow down as the day goes on and
US wakes up, so by time you run QEMU CI a second time in the day, it will
be slower. They clearly overcommit resources on the cloud host so you're
at the mercy of whatever else is running.

Regards,
Daniel
Paolo Bonzini May 18, 2021, 12:48 p.m. UTC | #5
On 18/05/21 14:43, Daniel P. Berrangé wrote:
> I'd be surprised if you can measure any statistically reliable difference
> at all wrt public CI. I've tried measuring CI performance for small changes
> and found it impossible in short time frames, as the deviation between runs
> is way too large. GitLab CI speeds tend to slow down as the day goes on and
> US wakes up, so by time you run QEMU CI a second time in the day, it will
> be slower. They clearly overcommit resources on the cloud host so you're
> at the mercy of whatever else is running.

Yeah, I was going to test it locally (using CPU offlining and hugetlbfs 
to simulate a 4-CPU machine with not that much memory).

Paolo
diff mbox series

Patch

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 4bd1a91aa8..3f0d86cf0a 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -17,7 +17,7 @@  include:
   stage: build
   image: $CI_REGISTRY_IMAGE/qemu/$IMAGE:latest
   before_script:
-    - JOBS=$(expr $(nproc) + 1)
+    - JOBS=$(nproc || echo 1)
   script:
     - if test -n "$LD_JOBS";
       then