[PATCH] OvmfPkg: enable multiprocessor builds when using build.sh

[PATCH] OvmfPkg: enable multiprocessor builds when using build.sh

[rebecca]

When building both BaseTools and OvmfPkg, enable multiprocessor builds,
using up to the number of cores available in the system. This can
drastically reduce build times.
For example, on a modern ThreadRipper system the
time required to build decreases from 3 minutes to 1 minute.

Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
---
 OvmfPkg/build.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/build.sh b/OvmfPkg/build.sh
index 4fcbdd2bc9..5d3a672bd2 100755
--- a/OvmfPkg/build.sh
+++ b/OvmfPkg/build.sh
@@ -40,7 +40,7 @@ ARCH_X64=no
 BUILDTARGET=DEBUG
 BUILD_OPTIONS=
 PLATFORMFILE=
-THREADNUMBER=1
+THREADNUMBER=$(getconf _NPROCESSORS_ONLN)
 LAST_ARG=
 RUN_QEMU=no
 ENABLE_FLASH=no
-- 
2.22.0

Re: [PATCH] OvmfPkg: enable multiprocessor builds when using build.sh

[Jordan Justen]

Maybe a commit message tweak would be:

OvmfPkg/build.sh: enable multithreaded build by default

On 2019-07-21 17:58:16, Rebecca Cran wrote:
> When building both BaseTools and OvmfPkg, enable multiprocessor builds,
> using up to the number of cores available in the system. This can
> drastically reduce build times.
> For example, on a modern ThreadRipper system the
> time required to build decreases from 3 minutes to 1 minute.
> 
> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> ---
>  OvmfPkg/build.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/build.sh b/OvmfPkg/build.sh
> index 4fcbdd2bc9..5d3a672bd2 100755
> --- a/OvmfPkg/build.sh
> +++ b/OvmfPkg/build.sh
> @@ -40,7 +40,7 @@ ARCH_X64=no
>  BUILDTARGET=DEBUG
>  BUILD_OPTIONS=
>  PLATFORMFILE=
> -THREADNUMBER=1
> +THREADNUMBER=$(getconf _NPROCESSORS_ONLN)

Based on OvmfPkg/build.sh --help, I think initializing THREADNUMBER to
0 might have the same effect, but not depend on getconf. Does that
work?

I'm not sure why I defaulted this to single threaded build way back in
578630802e.

It looks like if we tweaked things more, and omitted adding the -n
parameter to the build command by default, then it would use the
Conf/target.txt value, which by default appears to also be 0, so this
could accomplish the same thing, but also let a user set it in
target.txt.

-Jordan

>  LAST_ARG=
>  RUN_QEMU=no
>  ENABLE_FLASH=no
> -- 
> 2.22.0
> 

Re: [edk2-devel] [PATCH] OvmfPkg: enable multiprocessor builds when using build.sh

[Laszlo Ersek]

On 07/22/19 09:11, Jordan Justen wrote:
> Maybe a commit message tweak would be:
> 
> OvmfPkg/build.sh: enable multithreaded build by default
> 
> On 2019-07-21 17:58:16, Rebecca Cran wrote:
>> When building both BaseTools and OvmfPkg, enable multiprocessor builds,
>> using up to the number of cores available in the system. This can
>> drastically reduce build times.
>> For example, on a modern ThreadRipper system the
>> time required to build decreases from 3 minutes to 1 minute.
>>
>> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
>> ---
>>  OvmfPkg/build.sh | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/OvmfPkg/build.sh b/OvmfPkg/build.sh
>> index 4fcbdd2bc9..5d3a672bd2 100755
>> --- a/OvmfPkg/build.sh
>> +++ b/OvmfPkg/build.sh
>> @@ -40,7 +40,7 @@ ARCH_X64=no
>>  BUILDTARGET=DEBUG
>>  BUILD_OPTIONS=
>>  PLATFORMFILE=
>> -THREADNUMBER=1
>> +THREADNUMBER=$(getconf _NPROCESSORS_ONLN)
> 
> Based on OvmfPkg/build.sh --help, I think initializing THREADNUMBER to
> 0 might have the same effect, but not depend on getconf. Does that
> work?

My understanding is the same. The zero value causes the edk2 "build"
utility to fetch the logical CPU count with Python's
multiprocessing.cpu_count() method:

[Source/Python/build/build.py]

        if self.ThreadNumber == 0:
            try:
                self.ThreadNumber = multiprocessing.cpu_count()
            except (ImportError, NotImplementedError):
                self.ThreadNumber = 1

> I'm not sure why I defaulted this to single threaded build way back in
> 578630802e.

Your commit 578630802ee5 ("accept "-n THREADNUMBER" in OvmfPkg build
script", 2012-07-10) predates the now-default invocation of
multiprocessing.cpu_count() in "build.py".

The latter was added in 29af38b0f8f2 ("BaseTools: Enable
MAX_CONCURRENT_THREAD_NUMBER = 0 feature", 2018-01-15).

Therefore, your patch was consistent with the BaseTools behavior, at the
time you wrote it. We can consider Rebecca's patch to restore the
consistency between "OvmfPkg/build.sh" and the BaseTools default behavior.

> It looks like if we tweaked things more, and omitted adding the -n
> parameter to the build command by default, then it would use the
> Conf/target.txt value, which by default appears to also be 0, so this
> could accomplish the same thing, but also let a user set it in
> target.txt.

I assume that users prefer passing a simple command line parameter to
editing a text file. IOW I believe "THREADNUMBER=0" would be the best
solution.

Thanks
Laszlo

> 
> -Jordan
> 
>>  LAST_ARG=
>>  RUN_QEMU=no
>>  ENABLE_FLASH=no
>> -- 
>> 2.22.0
>>
> 
> 
> 

Re: [edk2-devel] [PATCH] OvmfPkg: enable multiprocessor builds when using build.sh

[Jordan Justen]

On 2019-07-22 13:06:03, Laszlo Ersek wrote:
> On 07/22/19 09:11, Jordan Justen wrote:
> 
> > It looks like if we tweaked things more, and omitted adding the -n
> > parameter to the build command by default, then it would use the
> > Conf/target.txt value, which by default appears to also be 0, so this
> > could accomplish the same thing, but also let a user set it in
> > target.txt.
> 
> I assume that users prefer passing a simple command line parameter to
> editing a text file. IOW I believe "THREADNUMBER=0" would be the best
> solution.
> 

TL;DR: I agree with you. :)

I think they prefer to do neither, and get the same result as "0". :)

I was suggesting that if they didn't specify -n as a param to
build.sh, then build.sh should not send -n to the edk2 build command.
The effect would be for the edk2 build command to check
Conf/target.txt. By default, I think target.txt will not set
THREADNUMBER, so "0" would still be the result.

Yet, it would give them the option to set it in Conf/target.txt.
Today, since we always use the -n param, target.txt is always ignored
for this parameter.

But, personally, I don't think the Conf directory is useful. I'd like
to see us deprecate it entirely, or at least make it optional.
Therefore, I think the best use of our time is to just set it to 0 as
you suggest. :)

-Jordan

Re: [edk2-devel] [PATCH] OvmfPkg: enable multiprocessor builds when using build.sh

[rebecca]

On 2019-07-22 17:14, Jordan Justen wrote:
>
> But, personally, I don't think the Conf directory is useful. I'd like
> to see us deprecate it entirely, or at least make it optional.
> Therefore, I think the best use of our time is to just set it to 0 as
> you suggest. :)


Thanks, I'll send a v2 patch in a while.


-- 
Rebecca Cran

Re: [edk2-devel] [PATCH] OvmfPkg: enable multiprocessor builds when using build.sh

[rebecca]

On 2019-07-22 17:14, Jordan Justen wrote:
>
> I was suggesting that if they didn't specify -n as a param to
> build.sh, then build.sh should not send -n to the edk2 build command.
> The effect would be for the edk2 build command to check
> Conf/target.txt. By default, I think target.txt will not set
> THREADNUMBER, so "0" would still be the result.
>
> Yet, it would give them the option to set it in Conf/target.txt.
> Today, since we always use the -n param, target.txt is always ignored
> for this parameter.


On a related topic, I wonder if we should add a "-j" parameter if we
build BaseTools for users (e.g. "make -j4 -C BaseTools")? I've found
that it can be pretty slow without it: on my system adding -j4 reduces
build time from 55 seconds to 15. Going higher doesn't seem to produce
much more benefit: -j32 (on a ThreadRipper system) reduces it to 12 seconds.


-- 
Rebecca Cran

Re: [edk2-devel] [PATCH] OvmfPkg: enable multiprocessor builds when using build.sh

[Laszlo Ersek]

On 07/23/19 02:00, Rebecca Cran wrote:
> On 2019-07-22 17:14, Jordan Justen wrote:
>>
>> I was suggesting that if they didn't specify -n as a param to
>> build.sh, then build.sh should not send -n to the edk2 build command.
>> The effect would be for the edk2 build command to check
>> Conf/target.txt. By default, I think target.txt will not set
>> THREADNUMBER, so "0" would still be the result.
>>
>> Yet, it would give them the option to set it in Conf/target.txt.
>> Today, since we always use the -n param, target.txt is always ignored
>> for this parameter.
> 
> 
> On a related topic, I wonder if we should add a "-j" parameter if we
> build BaseTools for users (e.g. "make -j4 -C BaseTools")? I've found
> that it can be pretty slow without it: on my system adding -j4 reduces
> build time from 55 seconds to 15. Going higher doesn't seem to produce
> much more benefit: -j32 (on a ThreadRipper system) reduces it to 12 seconds.
> 
> 

Passing

  -j $(getconf _NPROCESSORS_ONLN)

to "make" (for building BaseTools) makes sense, IMO.

Thanks
Laszlo

Re: [edk2-devel] [PATCH] OvmfPkg: enable multiprocessor builds when using build.sh

[Jordan Justen]

On 2019-07-23 00:44:06, Laszlo Ersek wrote:
> On 07/23/19 02:00, Rebecca Cran wrote:
> > On 2019-07-22 17:14, Jordan Justen wrote:
> >>
> >> I was suggesting that if they didn't specify -n as a param to
> >> build.sh, then build.sh should not send -n to the edk2 build command.
> >> The effect would be for the edk2 build command to check
> >> Conf/target.txt. By default, I think target.txt will not set
> >> THREADNUMBER, so "0" would still be the result.
> >>
> >> Yet, it would give them the option to set it in Conf/target.txt.
> >> Today, since we always use the -n param, target.txt is always ignored
> >> for this parameter.
> > 
> > 
> > On a related topic, I wonder if we should add a "-j" parameter if we
> > build BaseTools for users (e.g. "make -j4 -C BaseTools")? I've found
> > that it can be pretty slow without it: on my system adding -j4 reduces
> > build time from 55 seconds to 15. Going higher doesn't seem to produce
> > much more benefit: -j32 (on a ThreadRipper system) reduces it to 12 seconds.
> > 
> > 
> 
> Passing
> 
>   -j $(getconf _NPROCESSORS_ONLN)
> 
> to "make" (for building BaseTools) makes sense, IMO.

I guess the concern might be that we'll be running a bunch of
make invocations in parallel, each trying to spawn a compilation for
each thread. O(n^2) compilations. :)

In the make man-page for -j: "When make invokes a sub-make, all
instances of make will coordinate to run the specified number of jobs
at a time;", but I'm not sure if that's how `build -n` is implemented.
(With make...)

Since python writes the makefiles, it could be used instead of
getconf, right?

What we need is someone to make the ninja-build backend for BaseTools.
:)

-Jordan

Re: [edk2-devel] [PATCH] OvmfPkg: enable multiprocessor builds when using build.sh

[Laszlo Ersek]

On 07/23/19 10:05, Jordan Justen wrote:
> On 2019-07-23 00:44:06, Laszlo Ersek wrote:
>> On 07/23/19 02:00, Rebecca Cran wrote:
>>> On 2019-07-22 17:14, Jordan Justen wrote:
>>>>
>>>> I was suggesting that if they didn't specify -n as a param to
>>>> build.sh, then build.sh should not send -n to the edk2 build command.
>>>> The effect would be for the edk2 build command to check
>>>> Conf/target.txt. By default, I think target.txt will not set
>>>> THREADNUMBER, so "0" would still be the result.
>>>>
>>>> Yet, it would give them the option to set it in Conf/target.txt.
>>>> Today, since we always use the -n param, target.txt is always ignored
>>>> for this parameter.
>>>
>>>
>>> On a related topic, I wonder if we should add a "-j" parameter if we
>>> build BaseTools for users (e.g. "make -j4 -C BaseTools")? I've found
>>> that it can be pretty slow without it: on my system adding -j4 reduces
>>> build time from 55 seconds to 15. Going higher doesn't seem to produce
>>> much more benefit: -j32 (on a ThreadRipper system) reduces it to 12 seconds.
>>>
>>>
>>
>> Passing
>>
>>   -j $(getconf _NPROCESSORS_ONLN)
>>
>> to "make" (for building BaseTools) makes sense, IMO.
> 
> I guess the concern might be that we'll be running a bunch of
> make invocations in parallel, each trying to spawn a compilation for
> each thread. O(n^2) compilations. :)
> 
> In the make man-page for -j: "When make invokes a sub-make, all
> instances of make will coordinate to run the specified number of jobs
> at a time;", but I'm not sure if that's how `build -n` is implemented.
> (With make...)
> 
> Since python writes the makefiles, it could be used instead of
> getconf, right?
> 
> What we need is someone to make the ninja-build backend for BaseTools.
> :)

Wait, there are two separate topics here.


(1) The "make" command we are discussing here is responsible solely for
building the host-native BaseTools binaries, from the C and CPP sources
under BaseTools:

  make -C $WORKSPACE/BaseTools

This "make" command is not an ancestor of the actual firmware build.


(2) Regarding the invocation of the "build" utility from an outer
"make", such that you end up with "make -j" --> build --> "make":

That's something we don't do in edk2 per se, but we *do* do it in edk2's
bundling in QEMU. It raised some challenges, but ultimately, the
innermost "make" processes will inherit such an environment from the
outermost "make" that will enable a successful coordination between
them, regardless of "-n 0" used with "build" in the middle.

Basically, even if "build -n 0" starts twenty inner "make" processes,
the job server in the outer "make" will ensure that no more than, say,
4, inner "make" processes are active at the same time. (Assuming 20
logical processors in the system, and "-j4" passed to the outer "make".)

A related TianoCore BZ is:

  https://bugzilla.tianocore.org/show_bug.cgi?id=1607

--*--

Anyway, for "OvmfPkg/build.sh", item (2) is irrelevant; so changing

  make -C $WORKSPACE/BaseTools

to

  make -C $WORKSPACE/BaseTools -j $(getconf _NPROCESSORS_ONLN)

would be useful and safe.

Thanks
Laszlo