From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.100, mailfrom: steven.shi@intel.com) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by groups.io with SMTP; Fri, 14 Jun 2019 05:58:06 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 14 Jun 2019 05:58:05 -0700 X-ExtLoop1: 1 Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga003.jf.intel.com with ESMTP; 14 Jun 2019 05:58:05 -0700 Received: from fmsmsx124.amr.corp.intel.com (10.18.125.39) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.408.0; Fri, 14 Jun 2019 05:58:05 -0700 Received: from shsmsx107.ccr.corp.intel.com (10.239.4.96) by fmsmsx124.amr.corp.intel.com (10.18.125.39) with Microsoft SMTP Server (TLS) id 14.3.408.0; Fri, 14 Jun 2019 05:58:05 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.33]) by SHSMSX107.ccr.corp.intel.com ([169.254.9.173]) with mapi id 14.03.0439.000; Fri, 14 Jun 2019 20:58:03 +0800 From: "Steven Shi" To: "Rodriguez, Christian" , "devel@edk2.groups.io" CC: "Gao, Liming" , "Feng, Bob C" Subject: Re: [PATCH] BaseTools: Cannot store library cache of different arch together Thread-Topic: [PATCH] BaseTools: Cannot store library cache of different arch together Thread-Index: AQHVIBoH6t1g74NJIEupoDm7H5v6mKaWk3QwgADHHDCAAALlAIAAuNmQgAMKHUA= Date: Fri, 14 Jun 2019 12:58:01 +0000 Message-ID: <06C8AB66E78EE34A949939824ABE2B314001BEBF@shsmsx102.ccr.corp.intel.com> References: <20190611055327.18508-1-steven.shi@intel.com> <3A7DCC9A944C6149BF832E1C9B718ABC01F42EC0@ORSMSX114.amr.corp.intel.com> <06C8AB66E78EE34A949939824ABE2B3140015630@shsmsx102.ccr.corp.intel.com> <3A7DCC9A944C6149BF832E1C9B718ABC01F434D8@ORSMSX114.amr.corp.intel.com> In-Reply-To: <3A7DCC9A944C6149BF832E1C9B718ABC01F434D8@ORSMSX114.amr.corp.intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMzljN2ZmODItYTBjYy00OTFhLWE5NzctMGNmOTkxNzVkMTkyIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiYlJuYWNsWmNuTHBieHpGUDQ5SkRsNWZSdUJGV01sUStYaVFLZU9QYTlRcnNrRkRNekFoSWN0cENuek1JVEVLRSJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: steven.shi@intel.com Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_06C8AB66E78EE34A949939824ABE2B314001BEBFshsmsx102ccrcor_" --_000_06C8AB66E78EE34A949939824ABE2B314001BEBFshsmsx102ccrcor_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Christian, I think about BZ 1895 again, and your suggestion of "change the hash to inc= lude the arch" is better. I've sent the v2 patch to enhance it. BTW, the CopyFileOnChange() is still necessary for BZ 1894. Thanks Steven Shi Intel\SSG\FID\Firmware Infrastructure From: Rodriguez, Christian Sent: Wednesday, June 12, 2019 10:31 PM To: Shi, Steven ; devel@edk2.groups.io Cc: Gao, Liming ; Feng, Bob C Subject: RE: [PATCH] BaseTools: Cannot store library cache of different arc= h together Hi Steven, This looks good, thank you. Thanks, Christian From: Shi, Steven Sent: Tuesday, June 11, 2019 8:30 PM To: Rodriguez, Christian >; devel@edk2.groups.io Cc: Gao, Liming >; Feng, = Bob C > Subject: RE: [PATCH] BaseTools: Cannot store library cache of different arc= h together Sorry, the CopyFileOnChange() will ensure only once IO store/restore writin= g for each library. The extra IO read is ok. Thanks Steven Shi Intel\SSG\FID\Firmware Infrastructure From: Shi, Steven Sent: Wednesday, June 12, 2019 11:24 AM To: Rodriguez, Christian >; devel@edk2.groups.io Cc: Gao, Liming >; Feng, = Bob C > Subject: RE: [PATCH] BaseTools: Cannot store library cache of different arc= h together Hi Christian, For the extra IO accesses for duplicated library, I plan to introduce the C= opyFileOnChange() function to solve it. Below is the CopyFileOnChange() BZ,= and I haven't sent its patch yet. The CopyFileOnChange() will ensure only = once IO store/restore access for each library. To avoid duplicated library = access is critical not only for the performance, but also for the writing d= ata race in multi-threads. Basetool need CopyFileOnChange function to avoid cache file writing race in= multi-thread build https://bugzilla.tianocore.org/show_bug.cgi?id=3D1894 Thanks Steven Shi Intel\SSG\FID\Firmware Infrastructure > -----Original Message----- > From: Rodriguez, Christian > Sent: Tuesday, June 11, 2019 11:41 PM > To: Shi, Steven >; deve= l@edk2.groups.io > Cc: Gao, Liming >; Feng= , Bob C > > Subject: RE: [PATCH] BaseTools: Cannot store library cache of different a= rch > together > > The change looks good overall, but I'm concerned about the performance if > there are multiple of the same library used by different modules. In this= case > the library will be copied once per arch per module. I'm not sure if it w= ould > make a giant impact, but just something to think about because you would = be > doing extra IO accesses for each duplicate library autogen. > > Possible suggestion: > Is it possible to change the hash to include the arch? Or you can store a > unique tuple pair like (lib.arch, lib) in the set to reduce the libraries= being > copied to a unique subset. > > Thanks, > Christian > > >-----Original Message----- > >From: Shi, Steven > >Sent: Monday, June 10, 2019 10:53 PM > >To: devel@edk2.groups.io > >Cc: Gao, Liming >; Fen= g, Bob C > >>; Rodriguez, Christia= n > >> > >Subject: [PATCH] BaseTools: Cannot store library cache of different arch > >together > > > >https://bugzilla.tianocore.org/show_bug.cgi?id=3D1895 > > > >Build cache cannot store cache for the same library modules in different= arch > >together. E.g. Both the below IA32 and X64 arch BaseLib caches should ex= ist > >after build Ovmf3264, but now only the one in X64 arch exist. > >The reason is the current Basetool use a set() to same all library AutoG= en > >objects, but the different arch lib AutoGen objects have same __hash_ va= lue > >which comes from the lib MetaFile(The path of module file): > > def __hash__(self): > > return hash(self.MetaFile) > > > >So the different arch lib AutoGen objects are duplicated one to the set(= ) and > >only one can exist. This is why the Basetool can only store one arch cac= he for > >library. > >This patch avoid to use the set() as the container to save the library a= nd > >module objects because the objects might have same __hash__ value. > > > >Cc: Liming Gao > > >Cc: Bob Feng > > >Cc: Christian Rodriguez > > >Signed-off-by: Steven Shi > > >--- > > BaseTools/Source/Python/build/build.py | 13 +++---------- > > 1 file changed, 3 insertions(+), 10 deletions(-) > > > >diff --git a/BaseTools/Source/Python/build/build.py > >b/BaseTools/Source/Python/build/build.py > >index 0855d4561c..f7a79cbbab 100644 > >--- a/BaseTools/Source/Python/build/build.py > >+++ b/BaseTools/Source/Python/build/build.py > >@@ -2203,21 +2203,14 @@ class Build(): > > RemoveDirectory(os.path.dirname(GlobalData.gDatabasePath), = True) > > > > def CreateAsBuiltInf(self): > >- all_lib_set =3D set() > >- all_mod_set =3D set() > > for Module in self.BuildModules: > > Module.CreateAsBuiltInf() > >- all_mod_set.add(Module) > >+ for lib in Module.LibraryAutoGenList: > >+ lib.CreateAsBuiltInf(True) > > for Module in self.HashSkipModules: > > Module.CreateAsBuiltInf(True) > >- all_mod_set.add(Module) > >- for Module in all_mod_set: > > for lib in Module.LibraryAutoGenList: > >- all_lib_set.add(lib) > >- for lib in all_lib_set: > >- lib.CreateAsBuiltInf(True) > >- all_lib_set.clear() > >- all_mod_set.clear() > >+ lib.CreateAsBuiltInf(True) > > self.BuildModules =3D [] > > self.HashSkipModules =3D [] > > ## Do some clean-up works when error occurred > >-- > >2.17.1.windows.2 --_000_06C8AB66E78EE34A949939824ABE2B314001BEBFshsmsx102ccrcor_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable

Hi Christian,

I think about BZ 1895 = again, and your suggestion of “change the hash to include the arch= 221; is better. I’ve sent the v2 patch to enhance it.

BTW, the CopyFileOnChange() is still necessary for BZ 1894.<= /span>

 

 

Thanks

 

Steven Shi

Intel\SSG\FID\Firmware Infrastructur= e

 

From: Rod= riguez, Christian
Sent: Wednesday, June 12, 2019 10:31 PM
To: Shi, Steven <steven.shi@intel.com>; devel@edk2.groups.io Cc: Gao, Liming <liming.gao@intel.com>; Feng, Bob C <bob.c.= feng@intel.com>
Subject: RE: [PATCH] BaseTools: Cannot store library cache of differ= ent arch together

 

Hi Steven,<= /span>

 

This looks good, thank= you.

 

Thanks,

Christian

 

From: Shi, Steven
Sent: Tuesday, June 11, 2019 8:30 PM
To: Rodriguez, Christian <christian.rodriguez@intel.com>; devel@edk2.groups.io
Cc: Gao, Liming <liming.g= ao@intel.com>; Feng, Bob C <bob.c.feng@intel.com>
Subject: RE: [PATCH] BaseTools: Cannot store library cache of differ= ent arch together

 

Sorry, the CopyFileOnC= hange() will ensure only once IO store/restore writing for each library. Th= e extra IO read is ok.

 

 

Thanks

 

Steven Shi

Intel\SSG\FID\Firmware Infrastructur= e

 

From: Shi, Steven
Sent: Wednesday, June 12, 2019 11:24 AM
To: Rodriguez, Christian <christian.rodriguez@intel.com>; devel@edk2.groups.io
Cc: Gao, Liming <liming.g= ao@intel.com>; Feng, Bob C <bob.c.feng@intel.com>
Subject: RE: [PATCH] BaseTools: Cannot store library cache of differ= ent arch together

 

Hi Christian,

For the extra IO accesses for duplicated library,= I plan to introduce the CopyFileOnChange() function to solve it. Below is = the CopyFileOnChange() BZ, and I haven’t sent its patch yet. The Copy= FileOnChange() will ensure only once IO store/restore access for each library. To avoid duplicated library access = is critical not only for the performance, but also for the writing data rac= e in multi-threads.

 

 

Basetool need CopyFileOnChange function to avoid = cache file writing race in multi-thread build

https://bugzilla.tianocore.org/show_bug.cgi?id=3D1894

 

 

Thanks

 

Steven Shi

Intel\SSG\FID\Firmware Infrastructure<= /p>

 

> -----Original Message-----

> From: Rodriguez, Christian

> Sent: Tuesday, June 11, 2019 11:41 PM

> To: Shi, Steven <steven.shi@intel.com>; devel@edk2.groups.io=

> Cc: Gao, Liming <liming.gao@intel.com>; Feng, Bob C <bob.c.feng@intel.com>

> Subject: RE: [PATCH] BaseTools: Cannot store= library cache of different arch

> together

>

> The change looks good overall, but I'm conce= rned about the performance if

> there are multiple of the same library used = by different modules. In this case

> the library will be copied once per arch per= module. I'm not sure if it would

> make a giant impact, but just something to t= hink about because you would be

> doing extra IO accesses for each duplicate l= ibrary autogen.

>

> Possible suggestion:

> Is it possible to change the hash to include= the arch? Or you can store a

> unique tuple pair like (lib.arch, lib) in th= e set to reduce the libraries being

> copied to a unique subset.

>

> Thanks,

> Christian

>

> >-----Original Message-----

> >From: Shi, Steven

> >Sent: Monday, June 10, 2019 10:53 PM

> >To: devel@edk2.groups= .io

> >Cc: Gao, Liming <l= iming.gao@intel.com>; Feng, Bob C

> ><bob.c.feng@intel.= com>; Rodriguez, Christian

> ><christia= n.rodriguez@intel.com>

> >Subject: [PATCH] BaseTools: Cannot store= library cache of different arch

> >together

> >

> >https://bugzilla.tianocore.org/show_bug.cgi?id=3D1895<= /o:p>

> >

> >Build cache cannot store cache for the s= ame library modules in different arch

> >together. E.g. Both the below IA32 and X= 64 arch BaseLib caches should exist

> >after build Ovmf3264, but now only the o= ne in X64 arch exist.

> >The reason is the current Basetool use a= set() to same all library AutoGen

> >objects, but the different arch lib Auto= Gen objects have same __hash_ value

> >which comes from the lib MetaFile(The pa= th of module file):

> >    def __hash__(self):

> >      &nbs= p; return hash(self.MetaFile)

> >

> >So the different arch lib AutoGen object= s are duplicated one to the set() and

> >only one can exist. This is why the Base= tool can only store one arch cache for

> >library.

> >This patch avoid to use the set() as the= container to save the library and

> >module objects because the objects might= have same __hash__ value.

> >

> >Cc: Liming Gao <li= ming.gao@intel.com>

> >Cc: Bob Feng <bob.= c.feng@intel.com>

> >Cc: Christian Rodriguez <christian.rodriguez@intel.com>

> >Signed-off-by: Steven Shi <steven.shi@intel.com>

> >---

> > BaseTools/Source/Python/build/build.py = | 13 +++----------

> > 1 file changed, 3 insertions(+), 10= deletions(-)

> >

> >diff --git a/BaseTools/Source/Python/bui= ld/build.py

> >b/BaseTools/Source/Python/build/build.py=

> >index 0855d4561c..f7a79cbbab 100644=

> >--- a/BaseTools/Source/Python/build/buil= d.py

> >+++ b/BaseTools/Source/Pytho= n/build/build.py

> >@@ -2203,21 +2203,14 @@ class Build(= ):

> >      &nbs= p;      RemoveDirectory(os.path.dirname(GlobalData= .gDatabasePath), True)

> >

> >     def CreateAsBui= ltInf(self):

> >-      &nb= sp; all_lib_set =3D set()

> >-      &nb= sp; all_mod_set =3D set()

> >      &nbs= p;  for Module in self.BuildModules:

> >      &nbs= p;      Module.CreateAsBuiltInf()

> >-      &nb= sp;     all_mod_set.add(Module)

> >+      = ;      for lib in Module.LibraryAutoGenList:<= /o:p>

> >+      = ;          lib.CreateAsBuiltIn= f(True)

> >      &nbs= p;  for Module in self.HashSkipModules:

> >      &nbs= p;      Module.CreateAsBuiltInf(True)

> >-      &nb= sp;     all_mod_set.add(Module)

> >-      &nb= sp; for Module in all_mod_set:

> >      &nbs= p;      for lib in Module.LibraryAutoGenList:=

> >-      &nb= sp;         all_lib_set.add(lib)

> >-      &nb= sp; for lib in all_lib_set:

> >-      &nb= sp;     lib.CreateAsBuiltInf(True)

> >-      &nb= sp; all_lib_set.clear()

> >-      &nb= sp; all_mod_set.clear()

> >+      = ;          lib.CreateAsBuiltIn= f(True)

> >      &nbs= p;  self.BuildModules =3D []

> >      &nbs= p;  self.HashSkipModules =3D []

> >     ## Do some clea= n-up works when error occurred

> >--

> >2.17.1.windows.2

 

--_000_06C8AB66E78EE34A949939824ABE2B314001BEBFshsmsx102ccrcor_--