Re: [PATCH 1/8] ShellPkg/Comp: add file buffering

["Gao, Zhichao" <zhichao.gao@intel.com>]

Reviewed-by: Zhichao Gao <zhichao.gao@intel.com>

Thanks,
Zhichao

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Monday, January 4, 2021 11:42 PM
> To: edk2-devel-groups-io <devel@edk2.groups.io>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>; Ni, Ray <ray.ni@intel.com>;
> Gao, Zhichao <zhichao.gao@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@redhat.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Zhichao Gao <zhichao.gao@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3123
> Signed-off-by: Laszlo Ersek <lersek@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
>