Topics

[PATCH v2 01/10] ShellPkg/Comp: add file buffering


Laszlo Ersek
 

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>
Reviewed-by: Zhichao Gao <zhichao.gao@intel.com>
---

Notes:
v2:
- no changes
- pick up Zhichao's R-b

ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.inf | 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/UefiShellDebug1CommandsLib.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


Philippe Mathieu-Daudé
 

On 1/13/21 9:54 AM, Laszlo Ersek wrote:
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.
Wow o_O

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

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>
Reviewed-by: Zhichao Gao <zhichao.gao@intel.com>
---

Notes:
v2:
- no changes
- pick up Zhichao's R-b

ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.inf | 1 +
ShellPkg/Library/UefiShellDebug1CommandsLib/Comp.c | 127 +++++++++++++++++++-
2 files changed, 125 insertions(+), 3 deletions(-)