[edk2-devel] [Patch 00/11 V4] Enable multiple process AutoGen

[Bob Feng]

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 at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf Of Laszlo Ersek
Sent: Monday, July 29, 2019 6:10 PM
To: Feng, Bob C <bob.c.feng at intel.com>
Cc: devel at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-