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

x/sys/unix: add ParseDirent variant that doesn't allocate? #48161

Open
bradfitz opened this issue Sep 2, 2021 · 8 comments
Open

x/sys/unix: add ParseDirent variant that doesn't allocate? #48161

bradfitz opened this issue Sep 2, 2021 · 8 comments

Comments

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Sep 2, 2021

I recently wrote a function:

func CurrentOpenFDs() int { .... }

... to return how many file descriptors are open by the process. For Linux, the naive implementation is:

func CurrentOpenFDs() int {
	ents, _ := os.ReadDir("/proc/self/fd")
	return len(ents)
}

... but that allocates a bunch. Enough that it showed up disturbingly high in profiles. (worse: the more FDs open, the worse it is)

With some unsafe and Linux-specific stuff, you can get it down to zero allocations (e.g. tailscale/tailscale#2785).

Any objections to a unix.ParseDirent variant that parses dirents without allocating?

Then https://pkg.go.dev/golang.org/x/tools/internal/fastwalk could also use it.

/cc @ianlancetaylor @tklauser @crawshaw @dsnet

@cespare
Copy link
Contributor

@cespare cespare commented Sep 2, 2021

Yes, please. I wrote almost the same unsafe code several years ago for the same purpose (counting a process's FDs without a bajillion allocations).

Loading

@Xuanwo
Copy link

@Xuanwo Xuanwo commented Sep 3, 2021

Loading

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Sep 3, 2021

Loading

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 5, 2021

Seems fine in principle, but what's the API that you suggest?

Loading

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Sep 7, 2021

Seems fine in principle, but what's the API that you suggest?

Not exactly sure, but I'm sure it'll be slightly horrifying which is why I had to prepare y'all with this bug first.

Loading

@dsnet
Copy link
Member

@dsnet dsnet commented Sep 7, 2021

How about?

// ParseNextDirent parses the next directory entry,
// returning the directory name and the number of consumed bytes.
// It returns 0 if the input appears truncated and -1 if it is invalid.
func ParseNextDirent(b []byte) (name string, consumed int) {
    // NOTE: This function is inlineable so that the compiler can prove
    // based on how the caller uses the returned name argument
    // whether it needs to allocate or not.
    s, n := parseNextDirent(b)
    return string(s), n
}

func parseNextDirent(b []byte) (name []byte, consumed int)

For consistency with the existing ParseDirent function, we probably want it to return a string. However, to avoid allocations, we can rely on function outlining so that the compiler can avoid performing the []byte to string conversion for any cases that end up ignoring that argument.

Loading

@Xuanwo
Copy link

@Xuanwo Xuanwo commented Sep 8, 2021

The menory layout of Dirent is

type Dirent struct {
	Ino    uint64      // 64-bit inode number
	Off    int64       // 64-bit offset to next structure
	Reclen uint16      // Size of this dirent
	Type   uint8       // File type
	Name   [256]int8   // Filename (null-terminated)
	_      [5]byte     // Zero padding byte
}

So I think return name only is not enough, we need the Type too.

Loading

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Sep 8, 2021

@dsnet, I don't want any strings in the API. We already have the high-level nice API if you want strings.

And yes, we'd need the type and uint64 inode number.

Loading

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

Successfully merging a pull request may close this issue.

None yet
7 participants