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

In order to improve the build performance, we implemented
multiple-processes AutoGen. This change will reduce 20% time
for AutoGen phase.

The design document can be got from:
https://edk2.groups.io/g/devel/files/Designs/2019/0627/Multiple-thread-AutoGen.pdf

This patch serial pass the build of Ovmf, MinKabylake, MinPurley, packages
under Edk2 repository and intel client and server platforms.

V4:
Add one more patch 11/11 to enhance this feature. 1-10 are the same as V3
1. Set Log queue maxsize as thread number * 10
2. enhance ModuleUniqueBaseName function
3. fix bugs of build option pcd in sub Process
4. enhance error handling. Handle the exception of
KeyboardInterrup and exceptions happen in subprocess.
5. fix the issue of shared fixed pcd between module and lib.
6. fix bug in the function of duplicate modules handling.
V3:
1. Fixed incremental build issue.
2. Set AutoGen worker number to be align with "-n THREADNUMBER"
3. Enable block log queue.
V2: 
1. The first version missed autogen related commit 
from e812a812c1a0800c49e11507cb46222351520cc7. V2 add those commit
back.
2. Move CreateAsBuildInf into AutoGenWorker process
3. Save GlobalVar_<platform guid>_<arch>.bin to build folder.
4. Regenerate patches based on master bb824f685d
Feng, Bob C (11):
  BaseTools: Singleton the object to handle build conf file
  BaseTools: Split WorkspaceAutoGen._InitWorker into multiple functions
  BaseTools: Add functions to get platform scope build options
  BaseTools: Decouple AutoGen Objects
  BaseTools: Enable Multiple Process AutoGen
  BaseTools: Add shared data for processes
  BaseTools: Add LogAgent to support multiple process Autogen
  BaseTools: Move BuildOption parser out of build.py
  BaseTools: Add the support for python 2
  BaseTools: Enable block queue log agent.
  BaseTools: Enhance Multiple-Process AutoGen

 BaseTools/Source/Python/AutoGen/AutoGen.py    | 4227 +----------------
 .../Source/Python/AutoGen/AutoGenWorker.py    |  257 +
 .../Source/Python/AutoGen/BuildEngine.py      |   22 +
 BaseTools/Source/Python/AutoGen/DataPipe.py   |  160 +
 BaseTools/Source/Python/AutoGen/GenC.py       |    6 +-
 .../Source/Python/AutoGen/ModuleAutoGen.py    | 1903 ++++++++
 .../Python/AutoGen/ModuleAutoGenHelper.py     |  619 +++
 .../Source/Python/AutoGen/PlatformAutoGen.py  | 1512 ++++++
 .../Source/Python/AutoGen/WorkspaceAutoGen.py |  907 ++++
 BaseTools/Source/Python/Common/EdkLogger.py   |  119 +-
 BaseTools/Source/Python/Common/Misc.py        |    1 -
 .../Python/Common/TargetTxtClassObject.py     |   28 +-
 .../Python/Common/ToolDefClassObject.py       |    6 +-
 BaseTools/Source/Python/GenFds/GenFds.py      |    4 +-
 .../Python/GenFds/GenFdsGlobalVariable.py     |   54 +-
 .../Python/PatchPcdValue/PatchPcdValue.py     |    1 -
 .../Source/Python/Workspace/DscBuildData.py   |   38 +-
 .../Source/Python/Workspace/InfBuildData.py   |   39 +
 .../Python/Workspace/WorkspaceCommon.py       |    4 +
 .../Python/Workspace/WorkspaceDatabase.py     |    3 +
 BaseTools/Source/Python/build/BuildReport.py  |    4 +-
 BaseTools/Source/Python/build/build.py        |  370 +-
 BaseTools/Source/Python/build/buildoptions.py |   92 +
 23 files changed, 5921 insertions(+), 4455 deletions(-)
 create mode 100644 BaseTools/Source/Python/AutoGen/AutoGenWorker.py
 create mode 100644 BaseTools/Source/Python/AutoGen/DataPipe.py
 create mode 100644 BaseTools/Source/Python/AutoGen/ModuleAutoGen.py
 create mode 100644 BaseTools/Source/Python/AutoGen/ModuleAutoGenHelper.py
 create mode 100644 BaseTools/Source/Python/AutoGen/PlatformAutoGen.py
 create mode 100644 BaseTools/Source/Python/AutoGen/WorkspaceAutoGen.py
 create mode 100644 BaseTools/Source/Python/build/buildoptions.py

-- 
2.20.1.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44491): https://edk2.groups.io/g/devel/message/44491
Mute This Topic: https://groups.io/mt/32640226/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Bob,

On 07/29/19 10:44, Bob Feng wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1875
>
> In order to improve the build performance, we implemented
> multiple-processes AutoGen. This change will reduce 20% time
> for AutoGen phase.
>
> The design document can be got from:
> https://edk2.groups.io/g/devel/files/Designs/2019/0627/Multiple-thread-AutoGen.pdf
>
> This patch serial pass the build of Ovmf, MinKabylake, MinPurley,
> packages under Edk2 repository and intel client and server platforms.
>
> V4:
> Add one more patch 11/11 to enhance this feature. 1-10 are the same as
> V3
> 1. Set Log queue maxsize as thread number * 10
> 2. enhance ModuleUniqueBaseName function
> 3. fix bugs of build option pcd in sub Process
> 4. enhance error handling. Handle the exception of
> KeyboardInterrup and exceptions happen in subprocess.
> 5. fix the issue of shared fixed pcd between module and lib.
> 6. fix bug in the function of duplicate modules handling.

Item #1 seems to be in response to my v3 comment at:

  http://mid.mail-archive.com/e3f68d77-4837-0ef6-ab4f-95e50c4621ff@redhat.com
  https://edk2.groups.io/g/devel/message/44241

Therefore, I understand why item#1 is in scope for the v4 update.

However, the other updates in v4 (items #2 through #6) do not seem to
address review feedback for v3. I'm saying that because I cannot see
*any* feedback under v3 other than mine.

At

  http://mid.mail-archive.com/08650203BA1BD64D8AD9B6D5D74A85D160B468EB@SHSMSX105.ccr.corp.intel.com
  https://edk2.groups.io/g/devel/message/44249

you wrote,

> I'd like to collect more comments on other parts and update all the
> comments in V4.

Question#1: where are all those comments that justify the v4 updates
#2-#6? Did you get them in private (off-list)? Or did you determine the
necessity of #2-#6 yourself, without review feedback?

--*--

The following v4 updates are certainly bugfixes, relative to v3: #3, #5,
#6.

The following v4 updates *may* be bugfixes rather than additional
features ("enhancements") -- I can't tell myself, because they are not
explained deeply enough: #2, #4.

The point is, bugs that are known to be introduced by patches 01 through
10 should not be fixed up separately, in an incremental patch. Instead,
you should split out *minimally* the #3, #5, and #6 bugfixes, and squash
them into the appropriate patches between 01 and 10 (boundaries included
of course), please.

For example, regarding item #1, the following change from patch 11:

> -LogQMaxSize = 60
> +LogQMaxSize = ThreadNum() * 10

is wrong. Instead, you should update patch 10, so that when the log
agent is introduced, it be introduced at once with "LogQMaxSize =
ThreadNum() * 10".

The same applies to (minimally) #3, #5, and #6. Known bugs should not be
introduced mid-series, even temporarily. The bugs should be fixed up
inside the specific patches that introduce them in v3, and not in an
incremental patch.

If items #2 and #4 are indeed enhancements and not bugfixes (that is,
the series works fine without #2 / #4, functionally speaking, but #2/#4
improve some aspects, such as performance, user experience, etc), then
keeping them in separate patches might, or might not, make sense. That's
up to you, but even if you decide to separate them out of patches 01 to
10, they should still be isolated from *each other*.

Request#2: please restructure the patch series as explained.

--*--

The v4 cover letter, and patch v4 11/11, refer to a function called
"ModuleUniqueBaseName". I can't find the identifier
"ModuleUniqueBaseName" in the series.

Request#3: please clean up the cover letter and the commit messages. In
addition, please explain the v(n) --> v(n+1) updates in a lot more
detail, in the series cover letter. For example, item #5 seems like a
pretty serious bugfix, but nothing is explained about the nature of the
issue.

Thanks
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44506): https://edk2.groups.io/g/devel/message/44506
Mute This Topic: https://groups.io/mt/32640226/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Laszlo,

1. Question#1
I did not receive any except yours about the LogQ MaxSize. 2#-6# are introduced based on my testing result. They can be seen as bugfixes for regression issues. 
2. Request#2
Yes. I can restructure the patch series and sent out V5. I created a separate 11/11 patch because I thought it would be easier to tell what is my changes between V3 and V4. 
3. Request#3
Yes. I'll state more clear description in V5 cover letter.

My testing is to compare auto-gened files created by basetool with patches and without patches.
2#-6# resolved the regression issue for the following usage scenarios:
1. One module is built multiple times in one build. 2# and 6#
2. Exception handling. 1) The code run in sub-process raise exception. 2) Ctrl + C.  4#
3. --pcd is used in build command line. 3#
4. Shared fixedatbuild Pcd between module and its libraries. It affects the content of AutoGen.h and AutoGen.c. 5#
       
Thanks,
Bob

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek
Sent: Monday, July 29, 2019 6:10 PM
To: Feng, Bob C <bob.c.feng@intel.com>
Cc: devel@edk2.groups.io
Subject: Re: [edk2-devel] [Patch 00/11 V4] Enable multiple process AutoGen

Hi Bob,

On 07/29/19 10:44, Bob Feng wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1875
>
> In order to improve the build performance, we implemented 
> multiple-processes AutoGen. This change will reduce 20% time for 
> AutoGen phase.
>
> The design document can be got from:
> https://edk2.groups.io/g/devel/files/Designs/2019/0627/Multiple-thread
> -AutoGen.pdf
>
> This patch serial pass the build of Ovmf, MinKabylake, MinPurley, 
> packages under Edk2 repository and intel client and server platforms.
>
> V4:
> Add one more patch 11/11 to enhance this feature. 1-10 are the same as
> V3
> 1. Set Log queue maxsize as thread number * 10 2. enhance 
> ModuleUniqueBaseName function 3. fix bugs of build option pcd in sub 
> Process 4. enhance error handling. Handle the exception of 
> KeyboardInterrup and exceptions happen in subprocess.
> 5. fix the issue of shared fixed pcd between module and lib.
> 6. fix bug in the function of duplicate modules handling.

Item #1 seems to be in response to my v3 comment at:

  http://mid.mail-archive.com/e3f68d77-4837-0ef6-ab4f-95e50c4621ff@redhat.com
  https://edk2.groups.io/g/devel/message/44241

Therefore, I understand why item#1 is in scope for the v4 update.

However, the other updates in v4 (items #2 through #6) do not seem to address review feedback for v3. I'm saying that because I cannot see
*any* feedback under v3 other than mine.

At

  http://mid.mail-archive.com/08650203BA1BD64D8AD9B6D5D74A85D160B468EB@SHSMSX105.ccr.corp.intel.com
  https://edk2.groups.io/g/devel/message/44249

you wrote,

> I'd like to collect more comments on other parts and update all the 
> comments in V4.

Question#1: where are all those comments that justify the v4 updates #2-#6? Did you get them in private (off-list)? Or did you determine the necessity of #2-#6 yourself, without review feedback?

--*--

The following v4 updates are certainly bugfixes, relative to v3: #3, #5, #6.

The following v4 updates *may* be bugfixes rather than additional features ("enhancements") -- I can't tell myself, because they are not explained deeply enough: #2, #4.

The point is, bugs that are known to be introduced by patches 01 through
10 should not be fixed up separately, in an incremental patch. Instead, you should split out *minimally* the #3, #5, and #6 bugfixes, and squash them into the appropriate patches between 01 and 10 (boundaries included of course), please.

For example, regarding item #1, the following change from patch 11:

> -LogQMaxSize = 60
> +LogQMaxSize = ThreadNum() * 10

is wrong. Instead, you should update patch 10, so that when the log agent is introduced, it be introduced at once with "LogQMaxSize =
ThreadNum() * 10".

The same applies to (minimally) #3, #5, and #6. Known bugs should not be introduced mid-series, even temporarily. The bugs should be fixed up inside the specific patches that introduce them in v3, and not in an incremental patch.

If items #2 and #4 are indeed enhancements and not bugfixes (that is, the series works fine without #2 / #4, functionally speaking, but #2/#4 improve some aspects, such as performance, user experience, etc), then keeping them in separate patches might, or might not, make sense. That's up to you, but even if you decide to separate them out of patches 01 to 10, they should still be isolated from *each other*.

Request#2: please restructure the patch series as explained.

--*--

The v4 cover letter, and patch v4 11/11, refer to a function called "ModuleUniqueBaseName". I can't find the identifier "ModuleUniqueBaseName" in the series.

Request#3: please clean up the cover letter and the commit messages. In addition, please explain the v(n) --> v(n+1) updates in a lot more detail, in the series cover letter. For example, item #5 seems like a pretty serious bugfix, but nothing is explained about the nature of the issue.

Thanks
Laszlo




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44590): https://edk2.groups.io/g/devel/message/44590
Mute This Topic: https://groups.io/mt/32640226/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 07/30/19 09:31, Feng, Bob C wrote:
> Hi Laszlo,
> 
> 1. Question#1
> I did not receive any except yours about the LogQ MaxSize. 2#-6# are introduced based on my testing result. They can be seen as bugfixes for regression issues. 

OK, thanks. This information is very helpful. Please consider including
it in the v(n+1) cover letter, whenever it applies.

> 2. Request#2
> Yes. I can restructure the patch series and sent out V5. I created a separate 11/11 patch because I thought it would be easier to tell what is my changes between V3 and V4.

That's very thoughtful of you, thank you.

However, the goal of a patch series is to create good patches for the
git history (i.e. to make permanent stages of the project history as
self contained and as "perfect" as possible).

While reviewer burden *does* count, it is still secondary to the
self-containment of patches.

The right answer to the problem that you implicitly raise -- namely that
"incremental review of patch series is difficult" -- is to use proper
*tooling*. WebUI's are generally poor at this. However, the
"git-range-diff" command is quite good at it (that is, at displaying
"interdiff"s).

So please just update each patch properly, in order to remove the
regressions, and please leave it to reviewers to generate the resultant
interdiffs for review, with whatever tools they prefer.

> 3. Request#3
> Yes. I'll state more clear description in V5 cover letter.
> 
> My testing is to compare auto-gened files created by basetool with patches and without patches.

Nice!

> 2#-6# resolved the regression issue for the following usage scenarios:
> 1. One module is built multiple times in one build. 2# and 6#

Can you tell us more about this use case?

- Does it refer to the same library instance that is built into multiple
different drivers/applications, with module-scope PCD / LibClasses
overrides? (Such that PCDs / sub-libraries change how the library
instance behaves.)

- Or does it refer to multiple builds of the same driver/application,
with FILE_GUID overrides?

> 2. Exception handling. 1) The code run in sub-process raise exception. 2) Ctrl + C.  4#
> 3. --pcd is used in build command line. 3#
> 4. Shared fixedatbuild Pcd between module and its libraries. It affects the content of AutoGen.h and AutoGen.c. 5#

Thanks, these help me a bit to understand. In the v5 cover letter, can
you please repeat the explanations?

Thanks!
Laszlo

> Thanks,
> Bob
> 
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek
> Sent: Monday, July 29, 2019 6:10 PM
> To: Feng, Bob C <bob.c.feng@intel.com>
> Cc: devel@edk2.groups.io
> Subject: Re: [edk2-devel] [Patch 00/11 V4] Enable multiple process AutoGen
> 
> Hi Bob,
> 
> On 07/29/19 10:44, Bob Feng wrote:
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1875
>>
>> In order to improve the build performance, we implemented 
>> multiple-processes AutoGen. This change will reduce 20% time for 
>> AutoGen phase.
>>
>> The design document can be got from:
>> https://edk2.groups.io/g/devel/files/Designs/2019/0627/Multiple-thread
>> -AutoGen.pdf
>>
>> This patch serial pass the build of Ovmf, MinKabylake, MinPurley, 
>> packages under Edk2 repository and intel client and server platforms.
>>
>> V4:
>> Add one more patch 11/11 to enhance this feature. 1-10 are the same as
>> V3
>> 1. Set Log queue maxsize as thread number * 10 2. enhance 
>> ModuleUniqueBaseName function 3. fix bugs of build option pcd in sub 
>> Process 4. enhance error handling. Handle the exception of 
>> KeyboardInterrup and exceptions happen in subprocess.
>> 5. fix the issue of shared fixed pcd between module and lib.
>> 6. fix bug in the function of duplicate modules handling.
> 
> Item #1 seems to be in response to my v3 comment at:
> 
>   http://mid.mail-archive.com/e3f68d77-4837-0ef6-ab4f-95e50c4621ff@redhat.com
>   https://edk2.groups.io/g/devel/message/44241
> 
> Therefore, I understand why item#1 is in scope for the v4 update.
> 
> However, the other updates in v4 (items #2 through #6) do not seem to address review feedback for v3. I'm saying that because I cannot see
> *any* feedback under v3 other than mine.
> 
> At
> 
>   http://mid.mail-archive.com/08650203BA1BD64D8AD9B6D5D74A85D160B468EB@SHSMSX105.ccr.corp.intel.com
>   https://edk2.groups.io/g/devel/message/44249
> 
> you wrote,
> 
>> I'd like to collect more comments on other parts and update all the 
>> comments in V4.
> 
> Question#1: where are all those comments that justify the v4 updates #2-#6? Did you get them in private (off-list)? Or did you determine the necessity of #2-#6 yourself, without review feedback?
> 
> --*--
> 
> The following v4 updates are certainly bugfixes, relative to v3: #3, #5, #6.
> 
> The following v4 updates *may* be bugfixes rather than additional features ("enhancements") -- I can't tell myself, because they are not explained deeply enough: #2, #4.
> 
> The point is, bugs that are known to be introduced by patches 01 through
> 10 should not be fixed up separately, in an incremental patch. Instead, you should split out *minimally* the #3, #5, and #6 bugfixes, and squash them into the appropriate patches between 01 and 10 (boundaries included of course), please.
> 
> For example, regarding item #1, the following change from patch 11:
> 
>> -LogQMaxSize = 60
>> +LogQMaxSize = ThreadNum() * 10
> 
> is wrong. Instead, you should update patch 10, so that when the log agent is introduced, it be introduced at once with "LogQMaxSize =
> ThreadNum() * 10".
> 
> The same applies to (minimally) #3, #5, and #6. Known bugs should not be introduced mid-series, even temporarily. The bugs should be fixed up inside the specific patches that introduce them in v3, and not in an incremental patch.
> 
> If items #2 and #4 are indeed enhancements and not bugfixes (that is, the series works fine without #2 / #4, functionally speaking, but #2/#4 improve some aspects, such as performance, user experience, etc), then keeping them in separate patches might, or might not, make sense. That's up to you, but even if you decide to separate them out of patches 01 to 10, they should still be isolated from *each other*.
> 
> Request#2: please restructure the patch series as explained.
> 
> --*--
> 
> The v4 cover letter, and patch v4 11/11, refer to a function called "ModuleUniqueBaseName". I can't find the identifier "ModuleUniqueBaseName" in the series.
> 
> Request#3: please clean up the cover letter and the commit messages. In addition, please explain the v(n) --> v(n+1) updates in a lot more detail, in the series cover letter. For example, item #5 seems like a pretty serious bugfix, but nothing is explained about the nature of the issue.
> 
> Thanks
> Laszlo
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44629): https://edk2.groups.io/g/devel/message/44629
Mute This Topic: https://groups.io/mt/32640226/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-