From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Tue, 30 Jul 2019 07:02:45 -0700 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B5137307D95F; Tue, 30 Jul 2019 14:02:44 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (unknown [10.36.118.39]) by smtp.corp.redhat.com (Postfix) with ESMTP id E63FA19C67; Tue, 30 Jul 2019 14:02:43 +0000 (UTC) Subject: Re: [edk2-devel] [Patch 00/11 V4] Enable multiple process AutoGen To: "Feng, Bob C" , "devel@edk2.groups.io" References: <20190729084456.18844-1-bob.c.feng@intel.com> <55bec8ce-667e-a705-32b8-c5504ff1cfd8@redhat.com> <08650203BA1BD64D8AD9B6D5D74A85D160B4FC2E@SHSMSX105.ccr.corp.intel.com> From: "Laszlo Ersek" Message-ID: <5264dadb-4c22-9ba3-ba84-a76341153d07@redhat.com> Date: Tue, 30 Jul 2019 16:02:42 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <08650203BA1BD64D8AD9B6D5D74A85D160B4FC2E@SHSMSX105.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.48]); Tue, 30 Jul 2019 14:02:44 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > 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 > > >