Skip to content

Commit

Permalink
Fix performance regression in storage list.
Browse files Browse the repository at this point in the history 
storageListP() returns a list of entries in a path and should not need to stat/head, etc. in order to get more detailed info. This was broken by 75623d4 which failed to set the level correctly.

Set the correct level and update tests.

There's no easy way to directly test for a regression here but the SFTP tests will fail if more detailed info is requested since it would require script changes.
  • Loading branch information
@dwsteele
dwsteele committed Mar 7, 2024
1 parent 794c577 commit e00bfe2
Show file tree
Hide file tree
Showing 5 changed files with 37 additions and 16 deletions.
13 changes: 13 additions & 0 deletions doc/xml/release/2024/2.51.xml
View file
Expand Up @@ -14,6 +14,19 @@

<p>Skip zero-length files for block incremental delta restore.</p>
</release-item>

<release-item>
<github-issue id="2275"/>
<github-pull-request id="2276"/>

<release-item-contributor-list>
<release-item-ideator id="maksym.boguk"/>
<release-item-contributor id="david.steele"/>
<release-item-reviewer id="stephen.frost"/>
</release-item-contributor-list>

<p>Fix performance regression in storage list.</p>
</release-item>
</release-bug-list>

<release-improvement-list>
Expand Down
5 changes: 5 additions & 0 deletions doc/xml/release/contributor.xml
View file
Expand Up @@ -670,6 +670,11 @@
<contributor-id type="github">mahomedh</contributor-id>
</contributor>

<contributor id="maksym.boguk">
<contributor-name-display>Maksym Boguk</contributor-name-display>
<contributor-id type="github">MaximBoguk</contributor-id>
</contributor>

<contributor id="MannerMan">
<contributor-name-display>Oscar</contributor-name-display>
<contributor-id type="github">MannerMan</contributor-id>
Expand Down
4 changes: 2 additions & 2 deletions src/storage/storage.c
View file
Expand Up @@ -378,8 +378,8 @@ storageList(const Storage *this, const String *pathExp, StorageListParam param)
MEM_CONTEXT_TEMP_BEGIN()
{
StorageIterator *const storageItr = storageNewItrP(
this, pathExp, .errorOnMissing = param.errorOnMissing, .nullOnMissing = param.nullOnMissing,
.expression = param.expression);
this, pathExp, .level = storageInfoLevelExists, .errorOnMissing = param.errorOnMissing,
.nullOnMissing = param.nullOnMissing, .expression = param.expression);

if (storageItr != NULL)
{
Expand Down
17 changes: 17 additions & 0 deletions test/src/module/storage/posixTest.c
View file
Expand Up @@ -486,6 +486,23 @@ testRun(void)

storageTest->pub.interface.feature ^= 1 << storageFeatureInfoDetail;

// -------------------------------------------------------------------------------------------------------------------------
TEST_TITLE("path basic info - no recurse");

storageTest->pub.interface.feature ^= 1 << storageFeatureInfoDetail;

TEST_STORAGE_LIST(
storageTest, "pg",
"zzz/\n"
"pipe*\n"
"path/\n"
"link> {d=../file}\n"
"file {s=8, t=1656433838}\n"
"./\n",
.levelForce = true, .includeDot = true, .noRecurse = true, .sortOrder = sortOrderDesc);

storageTest->pub.interface.feature ^= 1 << storageFeatureInfoDetail;

// -------------------------------------------------------------------------------------------------------------------------
TEST_TITLE("empty path - filter");

Expand Down
14 changes: 0 additions & 14 deletions test/src/module/storage/sftpTest.c
View file
Expand Up @@ -3776,15 +3776,8 @@ testRun(void)
.resultInt = 8, .attrPerms = LIBSSH2_SFTP_S_IFREG | LIBSSH2_SFTP_S_IRWXU | LIBSSH2_SFTP_S_IRWXG,
.flags = LIBSSH2_SFTP_ATTR_PERMISSIONS | LIBSSH2_SFTP_ATTR_ACMODTIME | LIBSSH2_SFTP_ATTR_UIDGID,
.mtime = 1656434296, .uid = TEST_USER_ID, .gid = TEST_GROUP_ID},
{.function = HRNLIBSSH2_SFTP_STAT_EX, .param = "[\"" TEST_PATH "/.aaa.txt\",1]", .fileName = STRDEF(".aaa.txt"),
.resultInt = LIBSSH2_ERROR_NONE,
.attrPerms = LIBSSH2_SFTP_S_IFREG | LIBSSH2_SFTP_S_IRWXU | LIBSSH2_SFTP_S_IRWXG,
.flags = LIBSSH2_SFTP_ATTR_PERMISSIONS | LIBSSH2_SFTP_ATTR_ACMODTIME | LIBSSH2_SFTP_ATTR_UIDGID,
.mtime = 1656434296, .uid = TEST_USER_ID, .gid = TEST_GROUP_ID},
{.function = HRNLIBSSH2_SFTP_READDIR_EX, .param = "[\".aaa.txt\",4095,null,0]", .fileName = STRDEF("noperm"),
.resultInt = 6, .uid = 0, .gid = 0},
{.function = HRNLIBSSH2_SFTP_STAT_EX, .param = "[\"" TEST_PATH "/noperm\",1]", .fileName = STRDEF("noperm"),
.resultInt = LIBSSH2_ERROR_NONE, .uid = 0, .gid = 0},
{.function = HRNLIBSSH2_SFTP_READDIR_EX, .param = "[\"noperm\",4095,null,0]", .fileName = STRDEF(""),
.resultInt = LIBSSH2_ERROR_NONE, .uid = 0, .gid = 0},
{.function = HRNLIBSSH2_SFTP_CLOSE_HANDLE, .resultInt = LIBSSH2_ERROR_NONE},
Expand All @@ -3801,15 +3794,8 @@ testRun(void)
.resultInt = 7, .attrPerms = LIBSSH2_SFTP_S_IFREG | LIBSSH2_SFTP_S_IRWXU | LIBSSH2_SFTP_S_IRWXG,
.flags = LIBSSH2_SFTP_ATTR_PERMISSIONS | LIBSSH2_SFTP_ATTR_ACMODTIME | LIBSSH2_SFTP_ATTR_UIDGID,
.mtime = 1656434296, .uid = TEST_USER_ID, .gid = TEST_GROUP_ID},
{.function = HRNLIBSSH2_SFTP_STAT_EX, .param = "[\"" TEST_PATH "/bbb.txt\",1]", .fileName = STRDEF("bbb.txt"),
.resultInt = LIBSSH2_ERROR_NONE,
.attrPerms = LIBSSH2_SFTP_S_IFREG | LIBSSH2_SFTP_S_IRWXU | LIBSSH2_SFTP_S_IRWXG,
.flags = LIBSSH2_SFTP_ATTR_PERMISSIONS | LIBSSH2_SFTP_ATTR_ACMODTIME | LIBSSH2_SFTP_ATTR_UIDGID,
.mtime = 1656434296, .uid = TEST_USER_ID, .gid = TEST_GROUP_ID},
{.function = HRNLIBSSH2_SFTP_READDIR_EX, .param = "[\"bbb.txt\",4095,null,0]", .fileName = STRDEF("noperm"),
.resultInt = 6, .uid = 0, .gid = 0},
{.function = HRNLIBSSH2_SFTP_STAT_EX, .param = "[\"" TEST_PATH "/noperm\",1]", .fileName = STRDEF("noperm"),
.resultInt = LIBSSH2_ERROR_NONE, .uid = 0, .gid = 0},
{.function = HRNLIBSSH2_SFTP_READDIR_EX, .param = "[\"noperm\",4095,null,0]", .fileName = STRDEF(""),
.resultInt = LIBSSH2_ERROR_NONE, .uid = 0, .gid = 0},
{.function = HRNLIBSSH2_SFTP_CLOSE_HANDLE, .resultInt = LIBSSH2_ERROR_EAGAIN},
Expand Down

0 comments on commit e00bfe2

Please sign in to comment.