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

Merge common parts of process walker and connection walker #1011

Open
2opremio opened this issue Feb 23, 2016 · 3 comments
Open

Merge common parts of process walker and connection walker #1011

2opremio opened this issue Feb 23, 2016 · 3 comments
Labels
chore Related to fix/refinement/improvement of end user or new/existing developer functionality performance Excessive resource usage and latency; usually a bug or chore
Milestone

Comments

@2opremio
Copy link
Contributor

After #961 they are both going through /proc/PID/fd/* . See #812 (comment)

@2opremio 2opremio added the performance Excessive resource usage and latency; usually a bug or chore label Feb 25, 2016
@2opremio 2opremio added this to the 0.14.0 milestone Mar 4, 2016
@tomwilkie tomwilkie removed this from the 0.14.0 milestone Mar 17, 2016
@2opremio 2opremio modified the milestone: July2016 Jun 30, 2016
@rade rade added the tech-debt Unpleasantness that does (or may in future) affect development label Jul 4, 2016
@2opremio 2opremio modified the milestones: July2016, August2016 Aug 2, 2016
@rade rade modified the milestones: 0.18/1.0, October2016 Sep 15, 2016
@rade rade added chore Related to fix/refinement/improvement of end user or new/existing developer functionality and removed tech-debt Unpleasantness that does (or may in future) affect development labels Jan 11, 2017
@schu
Copy link
Contributor

schu commented Apr 19, 2017

I read over the code and don't see any obvious common parts between the two walkers:

While both do fs.ReadDirNames(path.Join(w.procRoot, filename, "fd")), pidWalker already uses the given CachingWalker to gather the namespace info (i.e. the data is already available). In walkNamespace, fs.ReadDirNames is used very specifically to associate sockets with processes.

Am I missing something?

@alban
Copy link
Contributor

alban commented Apr 20, 2017

probe/process/walker_linux.go calls ReadDirNames but it actually doesn't care about the names and only reports the number of files:

  openFiles, err := fs.ReadDirNames(path.Join(w.procRoot, filename, "fd"))
...
  OpenFilesCount: len(openFiles),

ReadDirNames calls syscall.Getdents and syscall.ParseDirent. The parsing and string allocations could be optimized since we don't care about the names.

I don't think that would change the performances much, but we could try.

@alban
Copy link
Contributor

alban commented Apr 20, 2017

With readdirnames.go, I ran 2000 iterations over /proc/*/fd/:

  • 12s using Golang's ReadDirNames
  • 8.6s using my new implementation without parsing the filenames, just counting the entries

The code takes a couple of assumptions on procfs:

  1. Linux always generates the "." and ".." directories.
  2. procfs' getdents does not returns missing files with inode=0

That's a 28% gain on my laptop with 340 processes running. It's better than I thought. I don't know how much it would help Scope (Scope does not do that many iterations over /proc/*/fd). And that would be more code to maintain.

/cc @ekimekim

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Related to fix/refinement/improvement of end user or new/existing developer functionality performance Excessive resource usage and latency; usually a bug or chore
Projects
None yet
Development

No branches or pull requests

5 participants