Re: [edk2-devel] [PATCH] BaseTools: Optimize GenerateByteArrayValue and CollectPlatformGuids APIs

["Feng, Bob C" <bob.c.feng@intel.com>]

Hi Ashraf,

Besides DSC, PCD value also comes from the build tool command line option, DEC and INF. This patch looks only check the PCD changes from DSC, it's not cover all the cases. 

Thanks,
Bob

-----Original Message-----
From: Kinney, Michael D <michael.d.kinney@intel.com> 
Sent: Tuesday, January 23, 2024 9:28 AM
To: S, Ashraf Ali <ashraf.ali.s@intel.com>; devel@edk2.groups.io
Cc: Chen, Christine <yuwei.chen@intel.com>; Rebecca Cran <rebecca@bsdio.com>; Liming Gao <gaoliming@byosoft.com.cn>; Feng, Bob C <bob.c.feng@intel.com>; Chan, Amy <amy.chan@intel.com>; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Solanki, Digant H <digant.h.solanki@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [PATCH] BaseTools: Optimize GenerateByteArrayValue and CollectPlatformGuids APIs

Hi Ashraf,

The PcdValueInit feature is not limited to only PCDs of type VPD.  It is for any structured PCDs.  Did you test PCDs with all types (e.g. PcdsFixedAtBuild PcdsPactahbleInModule, PcdsDynamicHii, PcdsDynamicDatabase, PcdsDynamicVpd, PcdsDynamicExHii, PcdsDynamicExDatabase, PcdsDynamicExVpd)

And did you DSC test cases cover both changes in PCD types and changes in PCD values?

Thanks,

Mike

> -----Original Message-----
> From: S, Ashraf Ali <ashraf.ali.s@intel.com>
> Sent: Sunday, January 21, 2024 4:14 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; 
> devel@edk2.groups.io
> Cc: Chen, Christine <yuwei.chen@intel.com>; Rebecca Cran 
> <rebecca@bsdio.com>; Liming Gao <gaoliming@byosoft.com.cn>; Feng, Bob 
> C <bob.c.feng@intel.com>; Chan, Amy <amy.chan@intel.com>; Chaganty, 
> Rangasai V <rangasai.v.chaganty@intel.com>; Solanki, Digant H 
> <digant.h.solanki@intel.com>
> Subject: RE: [edk2-devel] [PATCH] BaseTools: Optimize 
> GenerateByteArrayValue and CollectPlatformGuids APIs
> 
> Hi., @Kinney, Michael D
> 
> The main API which is modified in this Patch is GenerateByteArrayValue.
> 
> This API is used to return the list of 
> SKUID.TokenSpaceGuid.VpdName|VPD
> STRUCT|Binary Data which will be stored in Output.txt
> Example:
> SAMPLESKUID.STANDARD.gEfiMdePkgTokenSpaceGuid.SampleVpd|SAMPLE_STRUCT[
> ]|
> {0x01,0x01,0x05,0x09,0x02}
> 
> This VPD/PCD is coming from either the DEC file or the DSC file.
> 
> GenerateByteArrayValue API is used to create the PcdValueInit.exe and 
> it's equivalent C and Make File.
> 
> When there is no change in DEC or DSC VPD/PCD still it used to do the 
> same process again in the incremental build which is time consuming.
> In my project this API is used to take 3min of time with High end 
> server
> (64Threads) and 3.5Min in Laptop (16 threads with performance mode
> enabled) only for GenerateByteArrayValue API.
> 
> This  patch will check if there are any change in the DEC/DSC 
> VPDs/PCDs2, if there is any change in VPD the current flow is not 
> affect. (it will create the PcdRecordList.json and copy Output.txt for 
> Arch folder with the project. Ex:
> Build\{Project}Pkg\DEBUG_VS2019\PcdValueInit\{IA32/X64/Any}\PcdRecordL
> is
> t.json and
> Build\{Project}Pkg\DEBUG_VS2019\PcdValueInit\{IA32/X64/Any}\Output.txt
> ) If there is no change the DSC/DEC VPDs it will read the data from 
> Output.txt and return the same data. (we will compare the Input 
> structure and PcdRecordList.json) if there is any mismatch then it 
> there change in VPDs if not then no change in VPDs).
> 
> Unit testing from my side as follows:
> * Build the Firmware.
> 	* PcdRecordList.json and Output.txt will be created based on the 
> Arch.
> * Build the Firmware again without any change.
> 	* It will read the Output.txt and return the data. (it will match 
> Input PCD/VPD list and compare with existing PcdRecordList.json)
> 	* I verified the return length and content GenerateByteArrayValue 
> (it's same with and without my changes)
> * Build the Firmware again my modifying the VPDs in DSC file
> 	* Change in VPDs, it will regenerate PcdRecordList.json file
> 	*  it will create the Exe file and Output.txt
> 	* New Output.txt will be copied to above path.
> * Build the Firmware again by modifying the VPDs in DEC file.
> 	* Change in VPDs, it will regenerate PcdRecordList.json file
> 	*  it will create the Exe file and Output.txt
> 	* New Output.txt will be copied to above path.
> * Build the Firmware again without modifying the code.
> 	* It will read the Output.txt and return the data.
> 	* I verified the return length and content GenerateByteArrayValue 
> (it's same with and without my changes)
> 
> 
> There is no impact on the Boot/cache.
> This patch is used to reduce the incremental build time by checking if 
> the VPDs/PCDs are changed or not, if not changed then it will return 
> the previous stored data.
> 
> Thanks.,
> S, Ashraf Ali
> 
> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Saturday, January 20, 2024 7:36 AM
> To: devel@edk2.groups.io; S, Ashraf Ali <ashraf.ali.s@intel.com>
> Cc: Chen, Christine <yuwei.chen@intel.com>; Rebecca Cran 
> <rebecca@bsdio.com>; Liming Gao <gaoliming@byosoft.com.cn>; Feng, Bob 
> C <bob.c.feng@intel.com>; Chan, Amy <amy.chan@intel.com>; Chaganty, 
> Rangasai V <rangasai.v.chaganty@intel.com>; Solanki, Digant H 
> <digant.h.solanki@intel.com>; Kinney, Michael D 
> <michael.d.kinney@intel.com>
> Subject: RE: [edk2-devel] [PATCH] BaseTools: Optimize 
> GenerateByteArrayValue and CollectPlatformGuids APIs
> 
> Hi Ashraf,
> 
> What is captured in the file?
> 
> What PCD/VPD changes will invalidate the cache?  Just the number and 
> type of PCD/VPD elements or their default values/sizes?
> 
> How was this tested?  Were all conditions that invalidate the cache 
> tested?  I ask because incremental build is a very important feature 
> and if there is any logic error in the cache management of a file like 
> this, it will cause unexpected behavior and developers will not trust 
> incremental builds.
> 
> Mike
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of 
> > Ashraf Ali S
> > Sent: Tuesday, January 16, 2024 11:55 PM
> > To: devel@edk2.groups.io
> > Cc: S, Ashraf Ali <ashraf.ali.s@intel.com>; Chen, Christine 
> > <yuwei.chen@intel.com>; Rebecca Cran <rebecca@bsdio.com>; Liming Gao 
> > <gaoliming@byosoft.com.cn>; Feng, Bob C <bob.c.feng@intel.com>; 
> > Chan, Amy <amy.chan@intel.com>; Chaganty, Rangasai V 
> > <rangasai.v.chaganty@intel.com>; Solanki, Digant H 
> > <digant.h.solanki@intel.com>
> > Subject: [edk2-devel] [PATCH] BaseTools: Optimize 
> > GenerateByteArrayValue and CollectPlatformGuids APIs
> >
> > During the Incremental build GenerateByteArrayValue used to generate 
> > the ByteArrayValue even when there is no change in the PCD/VPDs. 
> > which is time consuming API based on the number of PCD/VPDs and SKU IDs.
> >
> > The optimization is that GenerateByteArrayValue is used to store the 
> > PcdRecordList in a JSON file for each of the arch. and during the 
> > Incremental build this API will check if there is any change in the 
> > PCD /VPDs then rest of the flow remains the same. if there is no 
> > change then it will return the provious build data.
> >
> > Flow:
> > during the 1st build PcdRecordList.json is not exists, PcdRecordList 
> > will be dumped to json file. and it will copy the output.txt as well.
> > Note: as the output.txt are different for different Arch, so it will 
> > be stored in the Arch folder.
> > During the Incremental build check if there is any change in PCD/VPD.
> > if there is a change in VPD/PCD then recreate the PcdRecordList.json.
> > and rest of the flow remains same.
> > if there is no change in VPD/PCD read the output.txt and return the 
> > data
> >
> > Cc: Yuwei Chen <yuwei.chen@intel.com>
> > Cc: Rebecca Cran <rebecca@bsdio.com>
> > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > Cc: Bob Feng <bob.c.feng@intel.com>
> > Cc: Amy Chan <amy.chan@intel.com>
> > Cc: Sai Chaganty <rangasai.v.chaganty@intel.com>
> > Cc: Digant H Solanki <digant.h.solanki@intel.com>
> > Signed-off-by: Ashraf Ali S <ashraf.ali.s@intel.com>
> > ---
> >  .../Source/Python/AutoGen/WorkspaceAutoGen.py | 16 ++---
> >  .../Source/Python/Workspace/DscBuildData.py   | 72 +++++++++++++++---
> -
> >  2 files changed, 64 insertions(+), 24 deletions(-)
> >
> > diff --git a/BaseTools/Source/Python/AutoGen/WorkspaceAutoGen.py
> > b/BaseTools/Source/Python/AutoGen/WorkspaceAutoGen.py
> > index 160e3a3cd3..eec9280c8e 100644
> > --- a/BaseTools/Source/Python/AutoGen/WorkspaceAutoGen.py
> > +++ b/BaseTools/Source/Python/AutoGen/WorkspaceAutoGen.py
> > @@ -160,22 +160,18 @@ class WorkspaceAutoGen(AutoGen):
> >
> >      def CollectPlatformGuids(self):
> >          oriInfList = []
> > -        oriPkgSet = set()
> > -        PlatformPkg = set()
> > +        pkgSet = set()
> >          for Arch in self.ArchList:
> >              Platform = self.BuildDatabase[self.MetaFile, Arch, 
> > self.BuildTarget, self.ToolChain]
> >              oriInfList = Platform.Modules
> >              for ModuleFile in oriInfList:
> >                  ModuleData = self.BuildDatabase[ModuleFile, 
> > Platform._Arch, Platform._Target, Platform._Toolchain]
> > -                oriPkgSet.update(ModuleData.Packages)
> > -                for Pkg in oriPkgSet:
> > -                    Guids = Pkg.Guids
> > -                    GlobalData.gGuidDict.update(Guids)
> > +                pkgSet.update(ModuleData.Packages)
> >              if Platform.Packages:
> > -                PlatformPkg.update(Platform.Packages)
> > -                for Pkg in PlatformPkg:
> > -                    Guids = Pkg.Guids
> > -                    GlobalData.gGuidDict.update(Guids)
> > +                pkgSet.update(Platform.Packages)
> > +        for Pkg in pkgSet:
> > +            Guids = Pkg.Guids
> > +            GlobalData.gGuidDict.update(Guids)
> >
> >      @cached_property
> >      def FdfProfile(self):
> > diff --git a/BaseTools/Source/Python/Workspace/DscBuildData.py
> > b/BaseTools/Source/Python/Workspace/DscBuildData.py
> > index 4768099343..740b8e22be 100644
> > --- a/BaseTools/Source/Python/Workspace/DscBuildData.py
> > +++ b/BaseTools/Source/Python/Workspace/DscBuildData.py
> > @@ -37,6 +37,8 @@ from functools import reduce  from Common.Misc 
> > import SaveFileOnChange  from Workspace.BuildClassObject import 
> > PlatformBuildClassObject, StructurePcd, PcdClassObject, 
> > ModuleBuildClassObject  from collections import OrderedDict, 
> > defaultdict
> > +import json
> > +import shutil
> >
> >  def _IsFieldValueAnArray (Value):
> >      Value = Value.strip()
> > @@ -56,6 +58,7 @@ def _IsFieldValueAnArray (Value):
> >
> >  PcdValueInitName = 'PcdValueInit'
> >  PcdValueCommonName = 'PcdValueCommon'
> > +PcdRecordListName = 'PcdRecordList.json'
> >
> >  PcdMainCHeader = '''
> >  /**
> > @@ -1599,7 +1602,7 @@ class DscBuildData(PlatformBuildClassObject):
> >          S_pcd_set = DscBuildData.OverrideByComm(S_pcd_set)
> >
> >          # Create a tool to caculate structure pcd value
> > -        Str_Pcd_Values = self.GenerateByteArrayValue(S_pcd_set)
> > +        Str_Pcd_Values = self.GenerateByteArrayValue(S_pcd_set,
> > RecordList)
> >
> >          if Str_Pcd_Values:
> >              for (skuname, StoreName, PcdGuid, PcdName, PcdValue) in
> > Str_Pcd_Values:
> > @@ -2750,12 +2753,61 @@ class DscBuildData(PlatformBuildClassObject):
> >                  ccflags.add(item)
> >              i +=1
> >          return ccflags
> > -    def GenerateByteArrayValue (self, StructuredPcds):
> > +
> > +    def GetStructurePcdSet (self, OutputValueFile):
> > +        if not os.path.isfile(OutputValueFile):
> > +            EdkLogger.error("GetStructurePcdSet", FILE_NOT_FOUND,
> > "Output.txt doesn't exist", ExtraData=OutputValueFile)
> > +            return []
> > +        File = open (OutputValueFile, 'r')
> > +        FileBuffer = File.readlines()
> > +        File.close()
> > +
> > +        #start update structure pcd final value
> > +        StructurePcdSet = []
> > +        for Pcd in FileBuffer:
> > +            PcdValue = Pcd.split ('|')
> > +            PcdInfo = PcdValue[0].split ('.')
> > +            StructurePcdSet.append((PcdInfo[0], PcdInfo[1], 
> > + PcdInfo[2],
> > PcdInfo[3], PcdValue[2].strip()))
> > +        return StructurePcdSet
> > +
> > +    def GenerateByteArrayValue (self, StructuredPcds, PcdRecordList):
> >          #
> >          # Generate/Compile/Run C application to determine if there 
> > are any flexible array members
> >          #
> >          if not StructuredPcds:
> >              return
> > +        #
> > +        # If the output path doesn't exists then create it
> > +        #
> > +        if not os.path.exists(self.OutputPath):
> > +            os.makedirs(self.OutputPath)
> > +
> > +        PcdRecordListPath = os.path.join(self.OutputPath, 
> > + self._Arch,
> > PcdRecordListName)
> > +        PcdRecordOutputValueFile = os.path.join(self.OutputPath,
> > self._Arch, 'Output.txt')
> > +
> > +        if not os.path.exists(os.path.dirname(PcdRecordListPath)):
> > +            os.makedirs(os.path.dirname(PcdRecordListPath))
> > +        #
> > +        # Check if the PcdRecordList.json exists or not
> > +        # if exits then it might be a incremental build then check 
> > + if
> > the PcdRecord list has been changed or not.
> > +        # if changed then proceed further, if not changed then 
> > + return
> > the stored data from earlier build
> > +        #
> > +        if os.path.isfile(PcdRecordListPath):
> > +            with open(PcdRecordListPath, 'r') as file:
> > +                file_content_str = file.read()
> > +                if file_content_str:
> > +                    # Use json.loads to convert the string back to 
> > + a
> > list
> > +                    file_content = json.loads(file_content_str)
> > +                    # Check if all PcdRecordList in record_set are
> > present in file_content
> > +                    # and if there are no extra PcdRecordList in
> > file_content
> > +                    if set(map(tuple, PcdRecordList)) == 
> > + set(map(tuple,
> > file_content)):
> > +                        return
> > self.GetStructurePcdSet(PcdRecordOutputValueFile)
> > +        #
> > +        # 1st build, create the PcdRecordList.json
> > +        # update the record as PCD Input has been changed in
> > incremental build
> > +        #
> > +        with open(PcdRecordListPath, 'w') as file:
> > +            json.dump(PcdRecordList, file)
> >
> >          InitByteValue = ""
> >          CApp = PcdMainCHeader
> > @@ -2832,8 +2884,6 @@ class DscBuildData(PlatformBuildClassObject):
> >
> >          CApp = CApp + PcdMainCEntry + '\n'
> >
> > -        if not os.path.exists(self.OutputPath):
> > -            os.makedirs(self.OutputPath)
> >          CAppBaseFileName = os.path.join(self.OutputPath,
> > PcdValueInitName)
> >          SaveFileOnChange(CAppBaseFileName + '.c', CApp, False)
> >
> > @@ -3042,17 +3092,11 @@ class DscBuildData(PlatformBuildClassObject):
> >              if returncode != 0:
> >                  EdkLogger.warn('Build', COMMAND_FAILURE, 'Can not 
> > collect output from command: %s\n%s\n%s\n' % (Command, StdOut,
> > StdErr))
> >
> > -        #start update structure pcd final value
> > -        File = open (OutputValueFile, 'r')
> > -        FileBuffer = File.readlines()
> > -        File.close()
> > +        # Copy output file for each Arch
> > +        shutil.copyfile(OutputValueFile, PcdRecordOutputValueFile)
> >
> > -        StructurePcdSet = []
> > -        for Pcd in FileBuffer:
> > -            PcdValue = Pcd.split ('|')
> > -            PcdInfo = PcdValue[0].split ('.')
> > -            StructurePcdSet.append((PcdInfo[0], PcdInfo[1],
> PcdInfo[2],
> > PcdInfo[3], PcdValue[2].strip()))
> > -        return StructurePcdSet
> > +        # return structure pcd final value
> > +        return self.GetStructurePcdSet(OutputValueFile)
> >
> >      @staticmethod
> >      def NeedUpdateOutput(OutputFile, ValueCFile, StructureInput):
> > --
> > 2.39.1.windows.1
> >
> >
> >
> > 
> >



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114176): https://edk2.groups.io/g/devel/message/114176
Mute This Topic: https://groups.io/mt/103781798/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-