Re: [Patch] BaseTools: Optimize string concatenation

["Gao, Liming" <liming.gao@intel.com>]

Leif:
  Agree your point to prepare the data before the patch instead of after the patch. 

Thanks
Liming
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Leif Lindholm
> Sent: Thursday, December 13, 2018 6:09 PM
> To: Gao, Liming <liming.gao@intel.com>
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel@lists.01.org
> Subject: Re: [edk2] [Patch] BaseTools: Optimize string concatenation
> 
> Hi Liming,
> 
> Yes, this is fine.
> But for future submissions, I would like for this sort of information
> to be provided at the time of posting.
> 
> Not the full breakdown, but "reduceces build time of platform XXX on
> hardware YYY by A%/B seconds".
> 
> Ideally, more than one platform and more than one hardware should be
> provided, but at least during this initial improvement phase I'm also
> happy for the assumption being that unless someone else complains,
> it's fine on others.
> 
> Regards,
> 
> Leif
> 
> On Thu, Dec 13, 2018 at 01:50:35AM +0000, Gao, Liming wrote:
> > Leif:
> >   Kabylake platform is the real Intel hardware.  The MinKabylake is the minimal feature of the Kabylake BIOS. Here is MinKabylake
> BIOS code https://github.com/tianocore/edk2-platforms/tree/devel-MinPlatform
> >   Bob adds the build performance data of MinKabylake into https://bugzilla.tianocore.org/show_bug.cgi?id=1288.
> >
> > The original build performance data:
> > Build Duration:       00:02:23
> > AutoGen Duration:     00:00:42
> > Make Duration:        00:01:12
> > GenFds Duration:      00:00:27
> >
> > After apply the patch, clean build performance is reduced from 2:23 to 1:57. So, I think his patch improves build performance.
> > Build Duration:       00:01:57
> > AutoGen Duration:     00:00:23
> > Make Duration:        00:01:12
> > GenFds Duration:      00:00:21
> >
> > Thanks
> > Liming
> > >-----Original Message-----
> > >From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Leif
> > >Lindholm
> > >Sent: Thursday, December 13, 2018 2:37 AM
> > >To: Feng, Bob C <bob.c.feng@intel.com>
> > >Cc: Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel@lists.01.org; Gao,
> > >Liming <liming.gao@intel.com>
> > >Subject: Re: [edk2] [Patch] BaseTools: Optimize string concatenation
> > >
> > >On Tue, Dec 11, 2018 at 08:48:19AM +0000, Feng, Bob C wrote:
> > >> Hi Leif,
> > >>
> > >> I understand your concern.
> > >>
> > >> I collected another performance data set based on open source
> > >> MinKabylake platform and updated the BZ
> > >> https://bugzilla.tianocore.org/show_bug.cgi?id=1288. The data looks
> > >> better than Ovmf. It enabled multiple SKU.
> > >>
> > >> Before I sent those patch, I did verify them on intel real
> > >> platforms. It improves the build performance. But it's not
> > >> convenient to share those data.
> > >
> > >So, I have two comments on this:
> > >1) How can it be inconvenient to share information on build times? I
> > >   don't even care what the names or codenames for those platforms
> > >   are. If you are unable to tell us why what you have done matters,
> > >   the code changes do not belong in the public tree.
> > >   Clearly having good performance numbers for public platforms is the
> > >   easiest solution for this problem.
> > >2) Submissions of improvements to build system performance should be
> > >   verified building real platforms. It should not be a question of
> > >   "find some other platform to get numbers from once we have improved
> > >   performance for building our confidential platforms".
> > >
> > >Regards,
> > >
> > >Leif
> > >>
> > >> Thanks,
> > >> Bob
> > >>
> > >> -----Original Message-----
> > >> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > >> Sent: Monday, December 10, 2018 8:36 PM
> > >> To: Feng, Bob C <bob.c.feng@intel.com>
> > >> Cc: edk2-devel@lists.01.org; Carsey, Jaben <jaben.carsey@intel.com>; Gao,
> > >Liming <liming.gao@intel.com>
> > >> Subject: Re: [edk2] [Patch] BaseTools: Optimize string concatenation
> > >>
> > >> On Mon, Dec 10, 2018 at 12:09:23PM +0000, Feng, Bob C wrote:
> > >> > For the "customized deepcopy" and "cache for uni file parser" data,
> > >> > you can see the AutoGen is not slower. The whole Build Duration is
> > >> > longer because Make Duration is longer while Make Duration time
> > >> > depends on the external make, compiler and linker. So it's not the
> > >> > patch make the build slow down.
> > >> >
> > >> > Yes,  it's not faster either. I think that because the Ovmf platform
> > >> > is relatively simple.  From the build tool source code point of view,
> > >> > the customized deepcopy will take effect if the platform enabled
> > >> > multiple SKU or there are many expressions in metadata file to be
> > >> > evaluated. And the "cache for uni file parser" needs there are many
> > >> > uni files.  The Ovmf platform looks not a good platform to demo the
> > >> > effect of this patch.
> > >>
> > >> But surely we should not introduce patches said to improve performance
> > >when the only data we have available shows that they slow things down?
> > >>
> > >> If the performance data is not representative, then it is worthless.
> > >>
> > >> Don't get me wrong - if you say "and for this secret platform I can't share
> > >with you, it improves build performance by X", then I may be OK with a minor
> > >slowdown on the platforms I do have available to test, if X is not minor.
> > >>
> > >> But if the improvement is only theoretical, and we have no evidence that it
> > >helps real platforms, it should not be committed.
> > >>
> > >> Regards,
> > >>
> > >> Leif
> > >_______________________________________________
> > >edk2-devel mailing list
> > >edk2-devel@lists.01.org
> > >https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel