improve find performance

[tw4452852]

use walkOnGoRoutine in any directory instead of root to improve concurrency.

[thomasf]

awesome!

[balta2ar]

@tw4452852 It is possible to add some performance benchmarks to see the difference it makes in actual numbers?

[thomasf]

Benchmarks depends very much on the latency of the file system used. For a ram disk, iirc the speed went up at least 20% in cpu usage as a result of less iowait.
A downside with this patch is that the order of search results becomes more random.

[tw4452852]

@balta2ar Sorry for late response. I did a test on my local, here is result:

I find all the makefile in a kernel source code tree,

with the original pt:
time pt_origin -g makefile "" . > /dev/null
pt_origin -g makefile "" . > /dev/null 7.57s user 0.27s system 80% cpu 9.772 total

with the improved pt:
time pt_new -g makefile "" . > /dev/null
pt_new -g makefile "" . > /dev/null 8.18s user 0.25s system 303% cpu 2.778 total

[thomasf]

generally I found that the increased disorder or results were not worth the increased speed.

[balta2ar]

Thank you for benchmarking this, @tw4452852.

What about adding this as an option? 9.772 -> 2.778 looks significant to me. What do you think?

[thomasf]

IIRC my test above was ran on a 1.5gb tmpfs, on my mechanical drives the difference was not noticeable at all. Depending on disk seek timings this change could potentially be slower as well (I don't have any data on this).

Making it optional and turned off by default is probably a good idea. It became really annoying using pt inside an Emacs window because every time I refreshed the results the order became radically different. For small searches an --ordered flag would perhaps also make sense even if it delays printing until all matches are calculated.

[balta2ar]

I'd go with an option. @tw4452852, could you implement it, please?

Making it optional and turned off by default is probably a good idea. It became really annoying using pt inside an Emacs window because every time I refreshed the results the order became radically different. For small searches an --ordered flag would perhaps also make sense even if it delays printing until all matches are calculated.

This sounds more like an #86 issue, which in fact can be extended, e.g. so that one can specify sort field like this --sort (date | name | extension | size).

[thomasf]

Yeah, these issues are related but not the same. I figured to mention the other issue as well.

[tw4452852]

@balta2ar Done.

[tw4452852]

Also another finding: in original code, the notify := make(chan int, len(list)) will be allocated in every function call, while it is only used in directRoot, so it adds a lot of overhead in gc. Now I just move it out of walk function as a *sync.waitGroup

[tw4452852]

It is faster now, also find all the Makefile in kernel source tree:

time pt --multi-finder -g Makefile . >/dev/null
pt --multi-finder -g Makefile . >/dev/null 5.79s user 0.27s system 682% cpu 0.887 total

[balta2ar]

Thank you very much, @tw4452852! Love your activity here! By the way, do you think it's worth adding unit tests for this option?

[tw4452852]

@balta2ar Of course. I have added a test for it.

[balta2ar]

Great job!

Guys, @tw4452852 @thomasf @monochromegane before this is set in stone, what if we think of some other name for this option? I'm not saying "--multi-finder" is bad and that we should change it, I'm just offering to think one last time to maybe find something even better and more intuitive. Naming things right matters a lot in my opinion. Please suggest your ideas, if any. Thanks!

[thomasf]

Ideally it should not use two words.. I guess that what it does can be described as maximizing IO throughput by being more concurrent. Something like --concurrent could maybe work, the help string would then need to clarify that it actually means increasing the concurrency rather than turning it on. I'll revisit this at the end of my day to see if I have other ideas then.

[padde]

How about --parallel? Just my 5¢.

[monochromegane]

Thank you for the PR, I check and merge this at this weekend. And I will release new version. 🍻
please wait :)

[balta2ar]

Thanks, @monochromegane!
Speaking of the option, I like --parallel better.

[thomasf]

Maybe an integer flag so that user can control it.. -maxpar N (?)

[monochromegane]

I checked, and thanks everyone !
I will merge this PR, and change option name to --parallel.

-maxpar N

I think this is a good idea, but now go-flags implementation can't represent the following

• pt (no option)
• pt --parallel (default value)
• pt --parallel=1 (specify value)

if you have a idea, please send a pull request.