Skip to content

Conversation

@shubhamdhama
Copy link
Contributor

@shubhamdhama shubhamdhama commented Nov 13, 2025

Previously, in a few places, we converted os.DirEntry to os.FileInfo by
calling entry.Info(), which triggers a stat syscall for each entry.

Most callers only needed the entry name, which is directly available from
os.DirEntry via Name(). This patch switches to os.DirEntry throughout the
codebase, avoiding unnecessary stat syscalls. In the few places where file
size or mode is needed, we now call Info() only when required.

Epic: none
Release note: None
Fixes: none

@shubhamdhama shubhamdhama requested review from a team as code owners November 13, 2025 13:26
@shubhamdhama shubhamdhama requested review from dhartunian and removed request for a team November 13, 2025 13:26
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@stevendanna stevendanna left a comment

Choose a reason for hiding this comment

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

I wonder: Would it be worth following up with a few of these callsites to see if they need to use this function at all?

I checked 2 of the call site and it appeared they only called Name() and IsDir() on the resulting FileInfo. Name and IsDir is available from the DirEntry's returned by the os.ReadDir call, so we could save some extra stat calls.

@shubhamdhama
Copy link
Contributor Author

@stevendanna, right, that's a good catch. I think I can do that clean-up as a follow-up for the security_assets.go and profilestore_test.go

Copy link
Collaborator

@stevendanna stevendanna left a comment

Choose a reason for hiding this comment

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

👍 This is a nice cleanup even without the follow-up.

@shubhamdhama shubhamdhama force-pushed the one-readdir-to-rule-them-all branch from 3ed0d4a to 5274559 Compare November 14, 2025 16:21
@shubhamdhama shubhamdhama requested a review from a team as a code owner November 14, 2025 16:21
@shubhamdhama shubhamdhama requested review from Abhinav1299, aa-joshi and arjunmahishi and removed request for a team November 14, 2025 16:21
@shubhamdhama
Copy link
Contributor Author

@stevendanna I pushed another commit following this suggestion: #157257 (review). This change makes fileutil.ReadDir redundant, so I've removed it. I'm planning to squash the first commit into the second (keeping only the second one) and update the PR title and description accordingly. Would appreciate another look when you get a chance.

Copy link
Collaborator

@stevendanna stevendanna left a comment

Choose a reason for hiding this comment

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

Thanks for following up!

@shubhamdhama shubhamdhama changed the title util: extract common ReadDir pattern to shared utility *: replace os.FileInfo with os.DirEntry where possible Nov 18, 2025
Previously, in a few places, we converted os.DirEntry to os.FileInfo by
calling entry.Info(), which triggers a stat syscall for each entry.

Most callers only needed the entry name, which is directly available from
os.DirEntry via Name(). This patch switches to os.DirEntry throughout the
codebase, avoiding unnecessary stat syscalls. In the few places where file
size or mode is needed, we now call Info() only when required.

Epic: none
Release note: None
Fixes: none
@shubhamdhama shubhamdhama force-pushed the one-readdir-to-rule-them-all branch from 5274559 to f663a2c Compare November 18, 2025 06:33
@shubhamdhama
Copy link
Contributor Author

TFTR!

bors r=stevendanna

@craig
Copy link
Contributor

craig bot commented Nov 18, 2025

@craig craig bot merged commit a384179 into cockroachdb:master Nov 18, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants