[edk2-devel] [PATCH 1/8] ShellPkg/Comp: add file buffering
Gao, Zhichao
zhichao.gao at intel.com
Mon Jan 11 01:24:04 UTC 2021
Reviewed-by: Zhichao Gao <zhichao.gao at intel.com>
Thanks,
Zhichao
> -----Original Message-----
> From: Laszlo Ersek <lersek at redhat.com>
> Sent: Monday, January 4, 2021 11:42 PM
> To: edk2-devel-groups-io <devel at edk2.groups.io>
> Cc: Philippe Mathieu-Daudé <philmd at redhat.com>; Ni, Ray <ray.ni at intel.com>;
> Gao, Zhichao <zhichao.gao at intel.com>
> Subject: [PATCH 1/8] ShellPkg/Comp: add file buffering
>
> The COMP shell command compares two files byte for byte. In order to retrieve
> the bytes to compare, it currently invokes
> gEfiShellProtocol->ReadFile() on both files, using a single-byte buffer
> every time. This is very inefficient; the underlying
> EFI_FILE_PROTOCOL.Read() function may be costly.
>
> Read both file operands in chunks of "PcdShellFileOperationSize" bytes.
> Draw bytes for comparison from the internal read-ahead buffers.
>
> Some ad-hoc measurements on my laptop, using OVMF, and the 4KB default of
> "PcdShellFileOperationSize":
>
> - When comparing two identical 1MB files that are served by EnhancedFatDxe
> on top of VirtioScsiDxe, this patch brings no noticeable improvement;
> the comparison completes in <1s both before and after.
>
> - When comparing two identical 1MB files served by VirtioFsDxe, the
> comparison time improves from 2 minutes 25 seconds to <1s.
>
> Cc: Philippe Mathieu-Daudé <philmd at redhat.com>
> Cc: Ray Ni <ray.ni at intel.com>
> Cc: Zhichao Gao <zhichao.gao at intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3123
> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
> ---
>
> ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.in
> f | 1 +
> ShellPkg/Library/UefiShellDebug1CommandsLib/Comp.c | 127
> +++++++++++++++++++-
> 2 files changed, 125 insertions(+), 3 deletions(-)
>
> diff --git
> a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib
> .inf
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib
> .inf
> index ed94477a0642..74ad5facf6b1 100644
> ---
> a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib
> .inf
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Command
> +++ sLib.inf
> @@ -113,6 +113,7 @@ [LibraryClasses]
> BcfgCommandLib
>
> [Pcd]
> + gEfiShellPkgTokenSpaceGuid.PcdShellFileOperationSize ## CONSUMES
> gEfiShellPkgTokenSpaceGuid.PcdShellProfileMask ## CONSUMES
>
> [Protocols]
> diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/Comp.c
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/Comp.c
> index 0a5d13f01c7b..0df0b3149369 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Comp.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Comp.c
> @@ -21,6 +21,16 @@ typedef enum {
> InPrevDiffPoint
> } READ_STATUS;
>
> +//
> +// Buffer type, for reading both file operands in chunks.
> +//
> +typedef struct {
> + UINT8 *Data; // dynamically allocated buffer
> + UINTN Allocated; // the allocated size of Data
> + UINTN Next; // next position in Data to fetch a byte at
> + UINTN Left; // number of bytes left in Data for fetching at Next
> +} FILE_BUFFER;
> +
> /**
> Function to print differnt point data.
>
> @@ -76,6 +86,106 @@ PrintDifferentPoint(
> ShellPrintEx (-1, -1, L"*\r\n");
> }
>
> +/**
> + Initialize a FILE_BUFFER.
> +
> + @param[out] FileBuffer The FILE_BUFFER to initialize. On return, the caller
> + is responsible for checking FileBuffer->Data: if
> + FileBuffer->Data is NULL on output, then memory
> + allocation failed.
> +**/
> +STATIC
> +VOID
> +FileBufferInit (
> + OUT FILE_BUFFER *FileBuffer
> + )
> +{
> + FileBuffer->Allocated = PcdGet32 (PcdShellFileOperationSize);
> + FileBuffer->Data = AllocatePool (FileBuffer->Allocated);
> + FileBuffer->Left = 0;
> +}
> +
> +/**
> + Uninitialize a FILE_BUFFER.
> +
> + @param[in,out] FileBuffer The FILE_BUFFER to uninitialize. The caller is
> + responsible for making sure FileBuffer was first
> + initialized with FileBufferInit(), successfully or
> + unsuccessfully.
> +**/
> +STATIC
> +VOID
> +FileBufferUninit (
> + IN OUT FILE_BUFFER *FileBuffer
> + )
> +{
> + SHELL_FREE_NON_NULL (FileBuffer->Data); }
> +
> +/**
> + Read a byte from a SHELL_FILE_HANDLE, buffered with a FILE_BUFFER.
> +
> + @param[in] FileHandle The SHELL_FILE_HANDLE to replenish FileBuffer
> + from, if needed.
> +
> + @param[in,out] FileBuffer The FILE_BUFFER to read a byte from. If FileBuffer
> + is empty on entry, then FileBuffer is refilled
> + from FileHandle, before outputting a byte from
> + FileBuffer to Byte. The caller is responsible for
> + ensuring that FileBuffer was successfully
> + initialized with FileBufferInit().
> +
> + @param[out] BytesRead On successful return, BytesRead is set to 1 if the
> + next byte from FileBuffer has been stored to Byte.
> + On successful return, BytesRead is set to 0 if
> + FileBuffer is empty, and FileHandle is at EOF.
> + When an error is returned, BytesRead is not set.
> +
> + @param[out] Byte On output, the next byte from FileBuffer. Only set
> + if (a) EFI_SUCCESS is returned and (b) BytesRead
> + is set to 1 on output.
> +
> + @retval EFI_SUCCESS BytesRead has been set to 0 or 1. In the latter case,
> + Byte has been set as well.
> +
> + @return Error codes propagated from
> + gEfiShellProtocol->ReadFile().
> +**/
> +STATIC
> +EFI_STATUS
> +FileBufferReadByte (
> + IN SHELL_FILE_HANDLE FileHandle,
> + IN OUT FILE_BUFFER *FileBuffer,
> + OUT UINTN *BytesRead,
> + OUT UINT8 *Byte
> + )
> +{
> + UINTN ReadSize;
> + EFI_STATUS Status;
> +
> + if (FileBuffer->Left == 0) {
> + ReadSize = FileBuffer->Allocated;
> + Status = gEfiShellProtocol->ReadFile (FileHandle, &ReadSize,
> + FileBuffer->Data);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> + if (ReadSize == 0) {
> + *BytesRead = 0;
> + return EFI_SUCCESS;
> + }
> + FileBuffer->Next = 0;
> + FileBuffer->Left = ReadSize;
> + }
> +
> + *BytesRead = 1;
> + *Byte = FileBuffer->Data[FileBuffer->Next];
> +
> + FileBuffer->Next++;
> + FileBuffer->Left--;
> + return EFI_SUCCESS;
> +}
> +
> /**
> Function for 'comp' command.
>
> @@ -107,6 +217,8 @@ ShellCommandRunComp (
> UINT8 OneByteFromFile2;
> UINT8 *DataFromFile1;
> UINT8 *DataFromFile2;
> + FILE_BUFFER FileBuffer1;
> + FILE_BUFFER FileBuffer2;
> UINTN InsertPosition1;
> UINTN InsertPosition2;
> UINTN DataSizeFromFile1;
> @@ -234,10 +346,15 @@ ShellCommandRunComp (
> if (ShellStatus == SHELL_SUCCESS) {
> DataFromFile1 = AllocateZeroPool ((UINTN)DifferentBytes);
> DataFromFile2 = AllocateZeroPool ((UINTN)DifferentBytes);
> - if (DataFromFile1 == NULL || DataFromFile2 == NULL) {
> + FileBufferInit (&FileBuffer1);
> + FileBufferInit (&FileBuffer2);
> + if (DataFromFile1 == NULL || DataFromFile2 == NULL ||
> + FileBuffer1.Data == NULL || FileBuffer2.Data == NULL) {
> ShellStatus = SHELL_OUT_OF_RESOURCES;
> SHELL_FREE_NON_NULL (DataFromFile1);
> SHELL_FREE_NON_NULL (DataFromFile2);
> + FileBufferUninit (&FileBuffer1);
> + FileBufferUninit (&FileBuffer2);
> }
> }
>
> @@ -247,9 +364,11 @@ ShellCommandRunComp (
> DataSizeFromFile2 = 1;
> OneByteFromFile1 = 0;
> OneByteFromFile2 = 0;
> - Status = gEfiShellProtocol->ReadFile (FileHandle1, &DataSizeFromFile1,
> &OneByteFromFile1);
> + Status = FileBufferReadByte (FileHandle1, &FileBuffer1,
> + &DataSizeFromFile1, &OneByteFromFile1);
> ASSERT_EFI_ERROR (Status);
> - Status = gEfiShellProtocol->ReadFile (FileHandle2, &DataSizeFromFile2,
> &OneByteFromFile2);
> + Status = FileBufferReadByte (FileHandle2, &FileBuffer2,
> + &DataSizeFromFile2, &OneByteFromFile2);
> ASSERT_EFI_ERROR (Status);
>
> TempAddress++;
> @@ -346,6 +465,8 @@ ShellCommandRunComp (
>
> SHELL_FREE_NON_NULL (DataFromFile1);
> SHELL_FREE_NON_NULL (DataFromFile2);
> + FileBufferUninit (&FileBuffer1);
> + FileBufferUninit (&FileBuffer2);
>
> if (DiffPointNumber == 0) {
> ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_COMP_FOOTER_PASS),
> gShellDebug1HiiHandle);
> --
> 2.19.1.3.g30247aa5d201
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70094): https://edk2.groups.io/g/devel/message/70094
Mute This Topic: https://groups.io/mt/79426445/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list