Re: [Patch] BaseTools: Optimize string concatenation

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

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