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

["Michael D Kinney" <michael.d.kinney@intel.com>]

Hi Christine,

I have created a PR with your Rb

https://github.com/tianocore/edk2/pull/5356

This review started before the soft freeze, so if it passes EDK II CI, 
I will merge it.

I do see that there are some issues with this same change being applied
to the edk2-basetools repo.  Please work with Ashraf to get those resolved
and keep the edk2-basetools and edk2/BaseTools repos aligned.

Thanks,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yuwei
> Chen
> Sent: Wednesday, February 7, 2024 6:36 PM
> To: S, Ashraf Ali <ashraf.ali.s@intel.com>; devel@edk2.groups.io
> Cc: 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 V4] BaseTools: Optimize
> GenerateByteArrayValue and CollectPlatformGuids APIs
> 
> Reviewed-by: Yuwei Chen <yuwei.chen@intel.com>
> 
> > -----Original Message-----
> > From: S, Ashraf Ali <ashraf.ali.s@intel.com>
> > Sent: Thursday, February 8, 2024 1:21 AM
> > 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: [PATCH V4] 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
> > StructuredPcdsData 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
> Structured
> > 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 StructuredPcdsData.json is not exists,
> StructuredPcdsData
> > 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
> Structured
> > PCD/VPD. if there is a change in Structured VPD/PCD then recreate the
> > StructuredPcdsData.json, and rest of the flow remains same.
> > if there is no change in VPD/PCD read the output.txt and return the
> data
> >
> > Unit Test:
> > Test1: Modified the Structured Pcds default from DEC file. current
> flow is
> > executing.
> > Test2: Override the default value of the PCD from DEC file. current
> flow is
> > executing.
> > Test3: Modified/Override the PCD from DSC file. current flow
> executing
> > Test4: Modified/Override the FDF from DSC file. current flow
> executing
> > Test5: update the default value from Command Line.current flow
> executing
> > Test6: Build without change in PCD in DSC, FDF, DEC and Command Line
> the
> > proposed changes will be executing, and the return data remains the
> same
> > with and without the changes.
> > Test7: Build with and without modified the include headers of
> Structured
> > PCDs. if there is any change in those Structured PCD header then
> current flow
> > will be executed.
> >
> > With these changes it's helping to save around ~2.5min to ~3.5min of
> > Incremental build time in my build environment.
> >
> > Sample PR: https://github.com/tianocore/edk2-basetools/pull/113
> >
> > 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   | 181 +++++++++++++---
> --
> >  2 files changed, 139 insertions(+), 58 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..ddcd157c73 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'
> > +StructuredPcdsDataName = 'StructuredPcdsData.json'
> >
> >  PcdMainCHeader = '''
> >  /**
> > @@ -2750,6 +2753,63 @@ class DscBuildData(PlatformBuildClassObject):
> >                  ccflags.add(item)
> >              i +=1
> >          return ccflags
> > +
> > +    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 GetBuildOptionsValueList(self):
> > +        CC_FLAGS = LinuxCFLAGS
> > +        if sys.platform == "win32":
> > +            CC_FLAGS = WindowsCFLAGS
> > +        BuildOptions = OrderedDict()
> > +        for Options in self.BuildOptions:
> > +            if Options[2] != EDKII_NAME:
> > +                continue
> > +            Family = Options[0]
> > +            if Family and Family != self.ToolChainFamily:
> > +                continue
> > +            Target, Tag, Arch, Tool, Attr = Options[1].split("_")
> > +            if Tool != 'CC':
> > +                continue
> > +            if Attr != "FLAGS":
> > +                continue
> > +            if Target == TAB_STAR or Target == self._Target:
> > +                if Tag == TAB_STAR or Tag == self._Toolchain:
> > +                    if 'COMMON' not in BuildOptions:
> > +                        BuildOptions['COMMON'] = set()
> > +                    if Arch == TAB_STAR:
> > +                        BuildOptions['COMMON']|=
> > self.ParseCCFlags(self.BuildOptions[Options])
> > +                    if Arch in self.SupArchList:
> > +                        if Arch not in BuildOptions:
> > +                            BuildOptions[Arch] = set()
> > +                        BuildOptions[Arch] |=
> > + self.ParseCCFlags(self.BuildOptions[Options])
> > +
> > +        if BuildOptions:
> > +            ArchBuildOptions = {arch:flags for arch,flags in
> BuildOptions.items() if
> > arch != 'COMMON'}
> > +            if len(ArchBuildOptions.keys()) == 1:
> > +                BuildOptions['COMMON'] |=
> (list(ArchBuildOptions.values())[0])
> > +            elif len(ArchBuildOptions.keys()) > 1:
> > +                CommonBuildOptions = reduce(lambda x,y: x&y,
> > ArchBuildOptions.values())
> > +                BuildOptions['COMMON'] |= CommonBuildOptions
> > +            ValueList = [item for item in BuildOptions['COMMON'] if
> > item.startswith((r"/U","-U"))]
> > +            ValueList.extend([item for item in
> BuildOptions['COMMON'] if
> > item.startswith((r"/D", "-D"))])
> > +            CC_FLAGS += " ".join(ValueList)
> > +        return CC_FLAGS
> > +
> > +
> >      def GenerateByteArrayValue (self, StructuredPcds):
> >          #
> >          # Generate/Compile/Run C application to determine if there
> are any
> > flexible array members @@ -2757,6 +2817,66 @@ class
> > DscBuildData(PlatformBuildClassObject):
> >          if not StructuredPcds:
> >              return
> >
> > +        StructuredPcdsData = {}
> > +        StoredStructuredPcdObjectPaths = {}
> > +        SkipPcdValueInit = False
> > +
> > +        CC_FLAGS = self.GetBuildOptionsValueList()
> > +
> > +        for PcdName in StructuredPcds:
> > +            Pcd = StructuredPcds[PcdName]
> > +            TokenSpaceGuidCName = Pcd.TokenSpaceGuidCName
> > +            TokenCName = Pcd.TokenCName
> > +
> > +            # Create a key using TokenSpaceGuidCName and TokenCName
> > +
> StructuredPcdsData[f"{TokenSpaceGuidCName}_{TokenCName}"] = {
> > +                "DefaultValueFromDec": Pcd.DefaultValueFromDec,
> > +                "DefaultValues": Pcd.DefaultValues,
> > +                "PcdFieldValueFromComm": Pcd.PcdFieldValueFromComm,
> > +                "PcdFieldValueFromFdf": Pcd.PcdFieldValueFromFdf,
> > +                "DefaultFromDSC": Pcd.DefaultFromDSC,
> > +                "PcdFiledValueFromDscComponent":
> > Pcd.PcdFiledValueFromDscComponent
> > +            }
> > +
> > +        # Store the CC Flags
> > +        StructuredPcdsData["CC_FLAGS"] = CC_FLAGS
> > +        #
> > +        # If the output path doesn't exists then create it
> > +        #
> > +        if not os.path.exists(self.OutputPath):
> > +            os.makedirs(self.OutputPath)
> > +
> > +        StructuredPcdsDataPath = os.path.join(self.OutputPath,
> self._Arch,
> > StructuredPcdsDataName)
> > +        PcdRecordOutputValueFile = os.path.join(self.OutputPath,
> > + self._Arch, 'Output.txt')
> > +
> > +        if not
> os.path.exists(os.path.dirname(StructuredPcdsDataPath)):
> > +            os.makedirs(os.path.dirname(StructuredPcdsDataPath))
> > +        #
> > +        # Check if the StructuredPcdsData.json exists or not
> > +        # if exits then it might be a incremental build then check
> if the
> > StructuredPcdsData 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(StructuredPcdsDataPath):
> > +            with open(StructuredPcdsDataPath, 'r') as file:
> > +                StoredStructuredPcdsData = json.load(file)
> > +                # OBJECTS will have the modified time, which needs
> to be checked
> > later
> > +                StoredStructuredPcdObjectPaths =
> > + StoredStructuredPcdsData.pop("OBJECTS", {})
> > +
> > +                if StructuredPcdsData == StoredStructuredPcdsData:
> > +                    SkipPcdValueInit = True
> > +                    for filename, file_mtime in
> > StoredStructuredPcdObjectPaths.items():
> > +                        f_mtime = os.path.getmtime(filename)
> > +                        #
> > +                        # check if the include_file are modified or
> not,
> > +                        # if modified then generate the PcdValueInit
> > +                        #
> > +                        if f_mtime != file_mtime:
> > +                            SkipPcdValueInit = False
> > +                            break
> > +
> > +        if SkipPcdValueInit:
> > +            return self.GetStructurePcdSet(PcdRecordOutputValueFile)
> > +
> >          InitByteValue = ""
> >          CApp = PcdMainCHeader
> >
> > @@ -2832,8 +2952,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)
> >
> > @@ -2890,42 +3008,6 @@ class DscBuildData(PlatformBuildClassObject):
> >                          IncSearchList.append(inc)
> >          MakeApp = MakeApp + '\n'
> >
> > -        CC_FLAGS = LinuxCFLAGS
> > -        if sys.platform == "win32":
> > -            CC_FLAGS = WindowsCFLAGS
> > -        BuildOptions = OrderedDict()
> > -        for Options in self.BuildOptions:
> > -            if Options[2] != EDKII_NAME:
> > -                continue
> > -            Family = Options[0]
> > -            if Family and Family != self.ToolChainFamily:
> > -                continue
> > -            Target, Tag, Arch, Tool, Attr = Options[1].split("_")
> > -            if Tool != 'CC':
> > -                continue
> > -            if Attr != "FLAGS":
> > -                continue
> > -            if Target == TAB_STAR or Target == self._Target:
> > -                if Tag == TAB_STAR or Tag == self._Toolchain:
> > -                    if 'COMMON' not in BuildOptions:
> > -                        BuildOptions['COMMON'] = set()
> > -                    if Arch == TAB_STAR:
> > -                        BuildOptions['COMMON']|=
> > self.ParseCCFlags(self.BuildOptions[Options])
> > -                    if Arch in self.SupArchList:
> > -                        if Arch not in BuildOptions:
> > -                            BuildOptions[Arch] = set()
> > -                        BuildOptions[Arch] |=
> > self.ParseCCFlags(self.BuildOptions[Options])
> > -
> > -        if BuildOptions:
> > -            ArchBuildOptions = {arch:flags for arch,flags in
> BuildOptions.items() if
> > arch != 'COMMON'}
> > -            if len(ArchBuildOptions.keys()) == 1:
> > -                BuildOptions['COMMON'] |=
> (list(ArchBuildOptions.values())[0])
> > -            elif len(ArchBuildOptions.keys()) > 1:
> > -                CommonBuildOptions = reduce(lambda x,y: x&y,
> > ArchBuildOptions.values())
> > -                BuildOptions['COMMON'] |= CommonBuildOptions
> > -            ValueList = [item for item in BuildOptions['COMMON'] if
> > item.startswith((r"/U","-U"))]
> > -            ValueList.extend([item for item in
> BuildOptions['COMMON'] if
> > item.startswith((r"/D", "-D"))])
> > -            CC_FLAGS += " ".join(ValueList)
> >          MakeApp += CC_FLAGS
> >
> >          if sys.platform == "win32":
> > @@ -2946,7 +3028,9 @@ class DscBuildData(PlatformBuildClassObject):
> >
> >
> SearchPathList.append(os.path.normpath(mws.join(GlobalData.gGlobalDefin
> > es["EDK_TOOLS_PATH"], "BaseTools/Source/C/Common")))
> >          SearchPathList.extend(str(item) for item in IncSearchList)
> >          IncFileList = GetDependencyList(IncludeFileFullPaths,
> SearchPathList)
> > +        StructuredPcdsData["OBJECTS"] = {}
> >          for include_file in IncFileList:
> > +            StructuredPcdsData["OBJECTS"][include_file] =
> > + os.path.getmtime(include_file)
> >              MakeApp += "$(OBJECTS) : %s\n" % include_file
> >          if sys.platform == "win32":
> >              PcdValueCommonPath =
> > os.path.normpath(mws.join(GlobalData.gGlobalDefines["EDK_TOOLS_PATH"]
> > , "Source\C\Common\PcdValueCommon.c"))
> > @@ -3042,17 +3126,18 @@ 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()
> > +        #
> > +        # In 1st build create the StructuredPcdsData.json
> > +        # update the record as PCD Input has been changed if its
> incremental
> > build
> > +        #
> > +        with open(StructuredPcdsDataPath, 'w') as file:
> > +            json.dump(StructuredPcdsData, file, indent=2)
> >
> > -        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
> > +        # Copy update output file for each Arch
> > +        shutil.copyfile(OutputValueFile, PcdRecordOutputValueFile)
> > +
> > +        #start update 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 (#115262): https://edk2.groups.io/g/devel/message/115262
Mute This Topic: https://groups.io/mt/104222804/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-