Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

small improvement around readdir(p) #4346

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

mykaul
Copy link
Contributor

@mykaul mykaul commented Apr 29, 2024

  • posix_readdirp_fill() does not need to copy or zero a gfid on every iteration: use an existing one or a pre-zero'ed one.
  • Make some functions static.
  • A bit of code restructure.

Updates: #1000

- posix_readdirp_fill() does not need to copy or zero a gfid on every iteration:
use an existing one or a pre-zero'ed one.
- Make some functions static.
- A bit of code restructure.

Updates: gluster#1000
Signed-off-by: Yaniv Kaul <[email protected]>
@mykaul
Copy link
Contributor Author

mykaul commented Apr 29, 2024

CC @jkroonza - this is a patch I've found somewhere in my archives and is an example of a (micro) optimization. HTH. I believe I've had one that improves some inode (link?) code paths that might be useful too.

@gluster-ant
Copy link
Collaborator

CLANG-FORMAT FAILURE:
Before merging the patch, this diff needs to be considered for passing clang-format

index ade8773fd..329f1e55c 100644
--- a/xlators/storage/posix/src/posix-inode-fd-ops.c
+++ b/xlators/storage/posix/src/posix-inode-fd-ops.c
@@ -5891,7 +5891,8 @@ posix_readdirp_fill(xlator_t *this, fd_t *fd, gf_dirent_t *entries,
             ret = posix_pstat(this, inode, inode->gfid, hpath, &stbuf,
                               _gf_false, _gf_true);
         else
-            ret = posix_pstat(this, inode, zero_gfid, hpath, &stbuf, _gf_false, _gf_true);
+            ret = posix_pstat(this, inode, zero_gfid, hpath, &stbuf, _gf_false,
+                              _gf_true);
 
         if (ret == -1) {
             if (inode)
@@ -6033,10 +6034,7 @@ posix_readdirp(call_frame_t *frame, xlator_t *this, fd_t *fd, size_t size,
         if (op_ret >= 0) {
             op_ret = 0;
 
-            list_for_each_entry(entry, &entries.list, list)
-            {
-                op_ret++;
-            }
+            list_for_each_entry(entry, &entries.list, list) { op_ret++; }
         }
 
         STACK_UNWIND_STRICT(readdirp, frame, op_ret, op_errno, &entries, NULL);

Copy link
Contributor

@jkroonza jkroonza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marked as approved for what it's worth. There is no logical changes here which took me a few rounds more than I'd care to count to verify.

static is the single bigger change here, and should result in a few calls being optimized.

As per discussion, these are marginal and may save a few ns per call, not ms or even us. I'd merge it for what it's worth anyway.

@mykaul
Copy link
Contributor Author

mykaul commented May 24, 2024

@aravindavk - I think 32bit build and testing is broken

@mykaul
Copy link
Contributor Author

mykaul commented May 24, 2024

/run regression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants