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

[Laszlo Ersek]

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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-