| Message ID | 1468252514-9533-3-git-send-email-lersek@redhat.com |
|---|---|
| State | Accepted |
| Commit | bd907fb6386560e621112beca7b7d381d0003967 |
| Headers | show |
On 07/11/16 19:01, Andrew Fish wrote: > >> On Jul 11, 2016, at 8:55 AM, Laszlo Ersek <lersek@redhat.com> wrote: >> >> The current implementation calls both pack() and Buffer.write() Size >> times. The new implementation calls both of these methods only once; the >> full data to write are constructed locally [1]. The range() function is >> replaced by xrange() because the latter is supposed to be faster / lighter >> weight [2]. >> >> On my laptop, I tested the change as follows: I pre-built the series at >> [3] with >> >> build -a X64 -p OvmfPkg/OvmfPkgX64.dsc -t GCC48 -b DEBUG \ >> -D HTTP_BOOT_ENABLE -D SECURE_BOOT_ENABLE >> >> (The series at [3] is relevant because it increases the size of one of the >> padded regions by 8.5 MB, slowing down the build quite a bit.) >> >> With all source code already compiled, repeating the above command takes >> approximately 45 seconds. With the patch applied, it goes down to 29 >> seconds. >> > > Laszlo, > > Thanks for tracking this down! > > It looks to me like this pattern is in 5 places in Region.py. Would it be cleaner to fix them all? Yes, that's exactly what this series does. The first patch in the series factors out this functionality into a tiny helper function (PadBuffer()), rebasing the five current call sites to it. Then the second patch optimizes the now-central function. Thanks! Laszlo > if Size > 0: > if (ErasePolarity == '1') : > PadData = 0xFF > else : > PadData = 0 > for i in range(0, Size): > Buffer.write(pack('B', PadData)) > > Thanks, > > Andrew Fish > > >> [1] http://stackoverflow.com/questions/27384093/fastest-way-to-write-huge-data-in-file >> [2] https://docs.python.org/2/library/functions.html?highlight=xrange#xrange >> [3] http://thread.gmane.org/gmane.comp.bios.edk2.devel/14214 >> >> We can also measure the impact with a synthetic test: >> >>> import timeit >>> >>> test_old = """ >>> import struct, string, StringIO >>> Size = (8 * 1024 + 512) * 1024 >>> Buffer = StringIO.StringIO() >>> PadData = 0xFF >>> for i in range(0, Size): >>> Buffer.write(struct.pack('B', PadData)) >>> """ >>> >>> test_new = """ >>> import struct, string, StringIO >>> Size = (8 * 1024 + 512) * 1024 >>> Buffer = StringIO.StringIO() >>> PadByte = struct.pack('B', 0xFF) >>> PadData = string.join(PadByte for i in xrange(0, Size)) >>> Buffer.write(PadData) >>> """ >>> >>> print(timeit.repeat(stmt=test_old, number=1, repeat=3)) >>> print(timeit.repeat(stmt=test_new, number=1, repeat=3)) >> >> The output is >> >> [8.231637001037598, 8.81188416481018, 8.948754072189331] >> [0.5503702163696289, 0.5461571216583252, 0.578315019607544] >> >> Cc: Yonghong Zhu <yonghong.zhu@intel.com> >> Cc: Liming Gao <liming.gao@intel.com> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> --- >> BaseTools/Source/Python/GenFds/Region.py | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/BaseTools/Source/Python/GenFds/Region.py b/BaseTools/Source/Python/GenFds/Region.py >> index 6769b39ba7e8..7548a4f014b1 100644 >> --- a/BaseTools/Source/Python/GenFds/Region.py >> +++ b/BaseTools/Source/Python/GenFds/Region.py >> @@ -18,6 +18,7 @@ >> from struct import * >> from GenFdsGlobalVariable import GenFdsGlobalVariable >> import StringIO >> +import string >> from CommonDataClass.FdfClass import RegionClassObject >> import Common.LongFilePathOs as os >> from stat import * >> @@ -52,11 +53,11 @@ class Region(RegionClassObject): >> def PadBuffer(self, Buffer, ErasePolarity, Size): >> if Size > 0: >> if (ErasePolarity == '1') : >> - PadData = 0xFF >> + PadByte = pack('B', 0xFF) >> else: >> - PadData = 0 >> - for i in range(0, Size): >> - Buffer.write(pack('B', PadData)) >> + PadByte = pack('B', 0) >> + PadData = string.join(PadByte for i in xrange(0, Size)) >> + Buffer.write(PadData) >> >> ## AddToBuffer() >> # >> -- >> 1.8.3.1 >> >> _______________________________________________ >> 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
On 07/12/16 07:18, Gao, Liming wrote: > Laszlo: > This patch serial is good. It will improve GenFds performance if there are more padding in FD region. > > Reviewed-by: Liming Gao <liming.gao@intel.com> Thanks! Your R-b applies to patch #1 in the series as well, correct? (Just to be sure before I commit it.) Thanks Laszlo >> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >> Laszlo Ersek >> Sent: Monday, July 11, 2016 11:55 PM >> To: edk2-devel-01 <edk2-devel@ml01.01.org> >> Cc: Gao, Liming <liming.gao@intel.com> >> Subject: [edk2] [PATCH 2/2] BaseTools/GenFds: speed up Region.PadBuffer() >> >> The current implementation calls both pack() and Buffer.write() Size >> times. The new implementation calls both of these methods only once; the >> full data to write are constructed locally [1]. The range() function is >> replaced by xrange() because the latter is supposed to be faster / lighter >> weight [2]. >> >> On my laptop, I tested the change as follows: I pre-built the series at >> [3] with >> >> build -a X64 -p OvmfPkg/OvmfPkgX64.dsc -t GCC48 -b DEBUG \ >> -D HTTP_BOOT_ENABLE -D SECURE_BOOT_ENABLE >> >> (The series at [3] is relevant because it increases the size of one of the >> padded regions by 8.5 MB, slowing down the build quite a bit.) >> >> With all source code already compiled, repeating the above command takes >> approximately 45 seconds. With the patch applied, it goes down to 29 >> seconds. >> >> [1] http://stackoverflow.com/questions/27384093/fastest-way-to-write- >> huge-data-in-file >> [2] >> https://docs.python.org/2/library/functions.html?highlight=xrange#xrange >> [3] http://thread.gmane.org/gmane.comp.bios.edk2.devel/14214 >> >> We can also measure the impact with a synthetic test: >> >>> import timeit >>> >>> test_old = """ >>> import struct, string, StringIO >>> Size = (8 * 1024 + 512) * 1024 >>> Buffer = StringIO.StringIO() >>> PadData = 0xFF >>> for i in range(0, Size): >>> Buffer.write(struct.pack('B', PadData)) >>> """ >>> >>> test_new = """ >>> import struct, string, StringIO >>> Size = (8 * 1024 + 512) * 1024 >>> Buffer = StringIO.StringIO() >>> PadByte = struct.pack('B', 0xFF) >>> PadData = string.join(PadByte for i in xrange(0, Size)) >>> Buffer.write(PadData) >>> """ >>> >>> print(timeit.repeat(stmt=test_old, number=1, repeat=3)) >>> print(timeit.repeat(stmt=test_new, number=1, repeat=3)) >> >> The output is >> >> [8.231637001037598, 8.81188416481018, 8.948754072189331] >> [0.5503702163696289, 0.5461571216583252, 0.578315019607544] >> >> Cc: Yonghong Zhu <yonghong.zhu@intel.com> >> Cc: Liming Gao <liming.gao@intel.com> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> --- >> BaseTools/Source/Python/GenFds/Region.py | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/BaseTools/Source/Python/GenFds/Region.py >> b/BaseTools/Source/Python/GenFds/Region.py >> index 6769b39ba7e8..7548a4f014b1 100644 >> --- a/BaseTools/Source/Python/GenFds/Region.py >> +++ b/BaseTools/Source/Python/GenFds/Region.py >> @@ -18,6 +18,7 @@ >> from struct import * >> from GenFdsGlobalVariable import GenFdsGlobalVariable >> import StringIO >> +import string >> from CommonDataClass.FdfClass import RegionClassObject >> import Common.LongFilePathOs as os >> from stat import * >> @@ -52,11 +53,11 @@ class Region(RegionClassObject): >> def PadBuffer(self, Buffer, ErasePolarity, Size): >> if Size > 0: >> if (ErasePolarity == '1') : >> - PadData = 0xFF >> + PadByte = pack('B', 0xFF) >> else: >> - PadData = 0 >> - for i in range(0, Size): >> - Buffer.write(pack('B', PadData)) >> + PadByte = pack('B', 0) >> + PadData = string.join(PadByte for i in xrange(0, Size)) >> + Buffer.write(PadData) >> >> ## AddToBuffer() >> # >> -- >> 1.8.3.1 >> >> _______________________________________________ >> 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
On 07/12/16 10:39, Laszlo Ersek wrote: > On 07/12/16 07:18, Gao, Liming wrote: >> Laszlo: >> This patch serial is good. It will improve GenFds performance if there are more padding in FD region. >> >> Reviewed-by: Liming Gao <liming.gao@intel.com> > > Thanks! > > Your R-b applies to patch #1 in the series as well, correct? (Just to be On a second thought, you did refer to the patch series in your message; plus the second patch doesn't make a lot of sense without the first one. So I'll take your R-b for the first patch as well. Series committed as 0f65154396df..bd907fb63865. Thanks! Laszlo >>> -----Original Message----- >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >>> Laszlo Ersek >>> Sent: Monday, July 11, 2016 11:55 PM >>> To: edk2-devel-01 <edk2-devel@ml01.01.org> >>> Cc: Gao, Liming <liming.gao@intel.com> >>> Subject: [edk2] [PATCH 2/2] BaseTools/GenFds: speed up Region.PadBuffer() >>> >>> The current implementation calls both pack() and Buffer.write() Size >>> times. The new implementation calls both of these methods only once; the >>> full data to write are constructed locally [1]. The range() function is >>> replaced by xrange() because the latter is supposed to be faster / lighter >>> weight [2]. >>> >>> On my laptop, I tested the change as follows: I pre-built the series at >>> [3] with >>> >>> build -a X64 -p OvmfPkg/OvmfPkgX64.dsc -t GCC48 -b DEBUG \ >>> -D HTTP_BOOT_ENABLE -D SECURE_BOOT_ENABLE >>> >>> (The series at [3] is relevant because it increases the size of one of the >>> padded regions by 8.5 MB, slowing down the build quite a bit.) >>> >>> With all source code already compiled, repeating the above command takes >>> approximately 45 seconds. With the patch applied, it goes down to 29 >>> seconds. >>> >>> [1] http://stackoverflow.com/questions/27384093/fastest-way-to-write- >>> huge-data-in-file >>> [2] >>> https://docs.python.org/2/library/functions.html?highlight=xrange#xrange >>> [3] http://thread.gmane.org/gmane.comp.bios.edk2.devel/14214 >>> >>> We can also measure the impact with a synthetic test: >>> >>>> import timeit >>>> >>>> test_old = """ >>>> import struct, string, StringIO >>>> Size = (8 * 1024 + 512) * 1024 >>>> Buffer = StringIO.StringIO() >>>> PadData = 0xFF >>>> for i in range(0, Size): >>>> Buffer.write(struct.pack('B', PadData)) >>>> """ >>>> >>>> test_new = """ >>>> import struct, string, StringIO >>>> Size = (8 * 1024 + 512) * 1024 >>>> Buffer = StringIO.StringIO() >>>> PadByte = struct.pack('B', 0xFF) >>>> PadData = string.join(PadByte for i in xrange(0, Size)) >>>> Buffer.write(PadData) >>>> """ >>>> >>>> print(timeit.repeat(stmt=test_old, number=1, repeat=3)) >>>> print(timeit.repeat(stmt=test_new, number=1, repeat=3)) >>> >>> The output is >>> >>> [8.231637001037598, 8.81188416481018, 8.948754072189331] >>> [0.5503702163696289, 0.5461571216583252, 0.578315019607544] >>> >>> Cc: Yonghong Zhu <yonghong.zhu@intel.com> >>> Cc: Liming Gao <liming.gao@intel.com> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >>> --- >>> BaseTools/Source/Python/GenFds/Region.py | 9 +++++---- >>> 1 file changed, 5 insertions(+), 4 deletions(-) >>> >>> diff --git a/BaseTools/Source/Python/GenFds/Region.py >>> b/BaseTools/Source/Python/GenFds/Region.py >>> index 6769b39ba7e8..7548a4f014b1 100644 >>> --- a/BaseTools/Source/Python/GenFds/Region.py >>> +++ b/BaseTools/Source/Python/GenFds/Region.py >>> @@ -18,6 +18,7 @@ >>> from struct import * >>> from GenFdsGlobalVariable import GenFdsGlobalVariable >>> import StringIO >>> +import string >>> from CommonDataClass.FdfClass import RegionClassObject >>> import Common.LongFilePathOs as os >>> from stat import * >>> @@ -52,11 +53,11 @@ class Region(RegionClassObject): >>> def PadBuffer(self, Buffer, ErasePolarity, Size): >>> if Size > 0: >>> if (ErasePolarity == '1') : >>> - PadData = 0xFF >>> + PadByte = pack('B', 0xFF) >>> else: >>> - PadData = 0 >>> - for i in range(0, Size): >>> - Buffer.write(pack('B', PadData)) >>> + PadByte = pack('B', 0) >>> + PadData = string.join(PadByte for i in xrange(0, Size)) >>> + Buffer.write(PadData) >>> >>> ## AddToBuffer() >>> # >>> -- >>> 1.8.3.1 >>> >>> _______________________________________________ >>> 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 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 12 July 2016 at 13:21, Laszlo Ersek <lersek@redhat.com> wrote: > On 07/12/16 10:39, Laszlo Ersek wrote: >> On 07/12/16 07:18, Gao, Liming wrote: >>> Laszlo: >>> This patch serial is good. It will improve GenFds performance if there are more padding in FD region. >>> >>> Reviewed-by: Liming Gao <liming.gao@intel.com> >> >> Thanks! >> >> Your R-b applies to patch #1 in the series as well, correct? (Just to be > > On a second thought, you did refer to the patch series in your message; > plus the second patch doesn't make a lot of sense without the first one. > So I'll take your R-b for the first patch as well. > > Series committed as 0f65154396df..bd907fb63865. > As a head's up, this patch appears to have broken ArmVirtQemu for me. I am investigating atm _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 07/12/16 15:28, Ard Biesheuvel wrote: > On 12 July 2016 at 13:21, Laszlo Ersek <lersek@redhat.com> wrote: >> On 07/12/16 10:39, Laszlo Ersek wrote: >>> On 07/12/16 07:18, Gao, Liming wrote: >>>> Laszlo: >>>> This patch serial is good. It will improve GenFds performance if there are more padding in FD region. >>>> >>>> Reviewed-by: Liming Gao <liming.gao@intel.com> >>> >>> Thanks! >>> >>> Your R-b applies to patch #1 in the series as well, correct? (Just to be >> >> On a second thought, you did refer to the patch series in your message; >> plus the second patch doesn't make a lot of sense without the first one. >> So I'll take your R-b for the first patch as well. >> >> Series committed as 0f65154396df..bd907fb63865. >> > > As a head's up, this patch appears to have broken ArmVirtQemu for me. Fantastic. I finally managed to let such a thing slip through my testing. Sigh. :( > I am investigating atm Thanks. I'm sorry. Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 07/12/16 15:34, Laszlo Ersek wrote: > On 07/12/16 15:28, Ard Biesheuvel wrote: >> On 12 July 2016 at 13:21, Laszlo Ersek <lersek@redhat.com> wrote: >>> On 07/12/16 10:39, Laszlo Ersek wrote: >>>> On 07/12/16 07:18, Gao, Liming wrote: >>>>> Laszlo: >>>>> This patch serial is good. It will improve GenFds performance if there are more padding in FD region. >>>>> >>>>> Reviewed-by: Liming Gao <liming.gao@intel.com> >>>> >>>> Thanks! >>>> >>>> Your R-b applies to patch #1 in the series as well, correct? (Just to be >>> >>> On a second thought, you did refer to the patch series in your message; >>> plus the second patch doesn't make a lot of sense without the first one. >>> So I'll take your R-b for the first patch as well. >>> >>> Series committed as 0f65154396df..bd907fb63865. >>> >> >> As a head's up, this patch appears to have broken ArmVirtQemu for me. > > Fantastic. I finally managed to let such a thing slip through my > testing. Sigh. :( > >> I am investigating atm > > Thanks. I'm sorry. Jesus how could I be so incredibly stupid. In commit bd907fb6386560e621112beca7b7d381d0003967, I used the string.join() method, but I forgot to set the separator to the empty string. https://docs.python.org/2/library/string.html#string.join The paddings now contain 0x20 as every second byte. I'll send a patch soon. Thanks for the report. Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 12 July 2016 at 15:48, Laszlo Ersek <lersek@redhat.com> wrote: > On 07/12/16 15:34, Laszlo Ersek wrote: >> On 07/12/16 15:28, Ard Biesheuvel wrote: >>> On 12 July 2016 at 13:21, Laszlo Ersek <lersek@redhat.com> wrote: >>>> On 07/12/16 10:39, Laszlo Ersek wrote: >>>>> On 07/12/16 07:18, Gao, Liming wrote: >>>>>> Laszlo: >>>>>> This patch serial is good. It will improve GenFds performance if there are more padding in FD region. >>>>>> >>>>>> Reviewed-by: Liming Gao <liming.gao@intel.com> >>>>> >>>>> Thanks! >>>>> >>>>> Your R-b applies to patch #1 in the series as well, correct? (Just to be >>>> >>>> On a second thought, you did refer to the patch series in your message; >>>> plus the second patch doesn't make a lot of sense without the first one. >>>> So I'll take your R-b for the first patch as well. >>>> >>>> Series committed as 0f65154396df..bd907fb63865. >>>> >>> >>> As a head's up, this patch appears to have broken ArmVirtQemu for me. >> >> Fantastic. I finally managed to let such a thing slip through my >> testing. Sigh. :( >> >>> I am investigating atm >> >> Thanks. I'm sorry. > > Jesus how could I be so incredibly stupid. > > In commit bd907fb6386560e621112beca7b7d381d0003967, I used the > string.join() method, but I forgot to set the separator to the empty string. > > https://docs.python.org/2/library/string.html#string.join > > The paddings now contain 0x20 as every second byte. > > I'll send a patch soon. Thanks for the report. > Indeed. The following hunk fixes it for me @@ -56,7 +57,7 @@ class Region(RegionClassObject): PadByte = pack('B', 0xFF) else: PadByte = pack('B', 0) - PadData = string.join(PadByte for i in xrange(0, Size)) + PadData = string.join((PadByte for i in xrange(0, Size)), '') Buffer.write(PadData) ## AddToBuffer() _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 12 July 2016 at 15:50, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 12 July 2016 at 15:48, Laszlo Ersek <lersek@redhat.com> wrote: [...] >> In commit bd907fb6386560e621112beca7b7d381d0003967, I used the >> string.join() method, but I forgot to set the separator to the empty string. >> >> https://docs.python.org/2/library/string.html#string.join >> >> The paddings now contain 0x20 as every second byte. >> >> I'll send a patch soon. Thanks for the report. >> > > Indeed. The following hunk fixes it for me > > @@ -56,7 +57,7 @@ class Region(RegionClassObject): > PadByte = pack('B', 0xFF) > else: > PadByte = pack('B', 0) > - PadData = string.join(PadByte for i in xrange(0, Size)) > + PadData = string.join((PadByte for i in xrange(0, Size)), '') > Buffer.write(PadData) > > ## AddToBuffer() ... or, given that string.join() is listed as deprecated, the following @@ -56,7 +57,7 @@ class Region(RegionClassObject): PadByte = pack('B', 0xFF) else: PadByte = pack('B', 0) - PadData = string.join(PadByte for i in xrange(0, Size)) + PadData = ''.join(PadByte for i in xrange(0, Size)) Buffer.write(PadData) ## AddToBuffer() _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 07/12/16 15:53, Ard Biesheuvel wrote: > On 12 July 2016 at 15:50, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> On 12 July 2016 at 15:48, Laszlo Ersek <lersek@redhat.com> wrote: > [...] >>> In commit bd907fb6386560e621112beca7b7d381d0003967, I used the >>> string.join() method, but I forgot to set the separator to the empty string. >>> >>> https://docs.python.org/2/library/string.html#string.join >>> >>> The paddings now contain 0x20 as every second byte. >>> >>> I'll send a patch soon. Thanks for the report. >>> >> >> Indeed. The following hunk fixes it for me >> >> @@ -56,7 +57,7 @@ class Region(RegionClassObject): >> PadByte = pack('B', 0xFF) >> else: >> PadByte = pack('B', 0) >> - PadData = string.join(PadByte for i in xrange(0, Size)) >> + PadData = string.join((PadByte for i in xrange(0, Size)), '') >> Buffer.write(PadData) >> >> ## AddToBuffer() > > ... or, given that string.join() is listed as deprecated, the following > > @@ -56,7 +57,7 @@ class Region(RegionClassObject): > PadByte = pack('B', 0xFF) > else: > PadByte = pack('B', 0) > - PadData = string.join(PadByte for i in xrange(0, Size)) > + PadData = ''.join(PadByte for i in xrange(0, Size)) > Buffer.write(PadData) > > ## AddToBuffer() > Thanks; this is the patch I'll send in a minute. Please respond with your R-b / T-b, and then I'll push it at once, considering it has the potential to break all platforms. ("Achievement unlocked" for me, I guess. :/) Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 07/12/16 16:06, Gao, Liming wrote: > Will you verify the generated file is the expected one? Yes, I compared SHA1 checksums for QEMU_EFI.fd (the build output of ArmVirtQemu.dsc). The first checksum is from the state of BaseTools at git commit 5588565f4845^ -- note the caret (^) at the end --, that is, directly before the first patch of my two-part series was applied. The second checksum is from the state of BaseTools with the fixup applied. The checksums are identical. I really should have done this earlier. I only tested OVMF and it worked :/ My apologies. Laszlo > *From:*Laszlo Ersek [mailto:lersek@redhat.com] > *Sent:* Tuesday, July 12, 2016 10:04 PM > *To:* Ard Biesheuvel <ard.biesheuvel@linaro.org> > *Cc:* Gao, Liming <liming.gao@intel.com>; edk2-devel-01 > <edk2-devel@ml01.01.org> > *Subject:* Re: [edk2] [PATCH 2/2] BaseTools/GenFds: speed up > Region.PadBuffer() > > > > On 07/12/16 15:53, Ard Biesheuvel wrote: >> On 12 July 2016 at 15:50, Ard Biesheuvel wrote: >>> On 12 July 2016 at 15:48, Laszlo Ersek wrote: >> [...] >>>> In commit bd907fb6386560e621112beca7b7d381d0003967, I used the >>>> string.join() method, but I forgot to set the separator to the empty string. >>>> >>>> https://docs.python.org/2/library/string.html#string.join >>>> >>>> The paddings now contain 0x20 as every second byte. >>>> >>>> I'll send a patch soon. Thanks for the report. >>>> >>> >>> Indeed. The following hunk fixes it for me >>> >>> @@ -56,7 +57,7 @@ class Region(RegionClassObject): >>> PadByte = pack('B', 0xFF) >>> else: >>> PadByte = pack('B', 0) >>> - PadData = string.join(PadByte for i in xrange(0, Size)) >>> + PadData = string.join((PadByte for i in xrange(0, Size)), '') >>> Buffer.write(PadData) >>> >>> ## AddToBuffer() >> >> ... or, given that string.join() is listed as deprecated, the following >> >> @@ -56,7 +57,7 @@ class Region(RegionClassObject): >> PadByte = pack('B', 0xFF) >> else: >> PadByte = pack('B', 0) >> - PadData = string.join(PadByte for i in xrange(0, Size)) >> + PadData = ''.join(PadByte for i in xrange(0, Size)) >> Buffer.write(PadData) >> >> ## AddToBuffer() >> > > Thanks; this is the patch I'll send in a minute. Please respond with > your R-b / T-b, and then I'll push it at once, considering it has the > potential to break all platforms. ("Achievement unlocked" for me, I > guess. :/) > > Laszlo > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/BaseTools/Source/Python/GenFds/Region.py b/BaseTools/Source/Python/GenFds/Region.py index 6769b39ba7e8..7548a4f014b1 100644 --- a/BaseTools/Source/Python/GenFds/Region.py +++ b/BaseTools/Source/Python/GenFds/Region.py @@ -18,6 +18,7 @@ from struct import * from GenFdsGlobalVariable import GenFdsGlobalVariable import StringIO +import string from CommonDataClass.FdfClass import RegionClassObject import Common.LongFilePathOs as os from stat import * @@ -52,11 +53,11 @@ class Region(RegionClassObject): def PadBuffer(self, Buffer, ErasePolarity, Size): if Size > 0: if (ErasePolarity == '1') : - PadData = 0xFF + PadByte = pack('B', 0xFF) else: - PadData = 0 - for i in range(0, Size): - Buffer.write(pack('B', PadData)) + PadByte = pack('B', 0) + PadData = string.join(PadByte for i in xrange(0, Size)) + Buffer.write(PadData) ## AddToBuffer() #
The current implementation calls both pack() and Buffer.write() Size times. The new implementation calls both of these methods only once; the full data to write are constructed locally [1]. The range() function is replaced by xrange() because the latter is supposed to be faster / lighter weight [2]. On my laptop, I tested the change as follows: I pre-built the series at [3] with build -a X64 -p OvmfPkg/OvmfPkgX64.dsc -t GCC48 -b DEBUG \ -D HTTP_BOOT_ENABLE -D SECURE_BOOT_ENABLE (The series at [3] is relevant because it increases the size of one of the padded regions by 8.5 MB, slowing down the build quite a bit.) With all source code already compiled, repeating the above command takes approximately 45 seconds. With the patch applied, it goes down to 29 seconds. [1] http://stackoverflow.com/questions/27384093/fastest-way-to-write-huge-data-in-file [2] https://docs.python.org/2/library/functions.html?highlight=xrange#xrange [3] http://thread.gmane.org/gmane.comp.bios.edk2.devel/14214 We can also measure the impact with a synthetic test: > import timeit > > test_old = """ > import struct, string, StringIO > Size = (8 * 1024 + 512) * 1024 > Buffer = StringIO.StringIO() > PadData = 0xFF > for i in range(0, Size): > Buffer.write(struct.pack('B', PadData)) > """ > > test_new = """ > import struct, string, StringIO > Size = (8 * 1024 + 512) * 1024 > Buffer = StringIO.StringIO() > PadByte = struct.pack('B', 0xFF) > PadData = string.join(PadByte for i in xrange(0, Size)) > Buffer.write(PadData) > """ > > print(timeit.repeat(stmt=test_old, number=1, repeat=3)) > print(timeit.repeat(stmt=test_new, number=1, repeat=3)) The output is [8.231637001037598, 8.81188416481018, 8.948754072189331] [0.5503702163696289, 0.5461571216583252, 0.578315019607544] Cc: Yonghong Zhu <yonghong.zhu@intel.com> Cc: Liming Gao <liming.gao@intel.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- BaseTools/Source/Python/GenFds/Region.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) -- 1.8.3.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel