search

calancha / dired-du

Dired with recursive directory sizes
GNU General Public License v3.0
9 stars 3 forks source link

dired-du-get-recursive-dir-size-in-parallel: Don't sleep for 1s #1

Closed Ambrevar closed 6 years ago

Ambrevar commented 6 years ago

I think dired-du is a great idea, but in practice I find it very frustrating to use because each folder takes 1s to open.

I've rooted out the slow-down to the dired-du-get-recursive-dir-size-in-parallel function: notice the (sleep-for 1) call there. If I remove it, it seems to be working still, but I suspect the sleep was introduced to avoid polling.

Changing it to (sleep-for 0 10) makes for a much better experience in my opinion.

That said, instead of polling for the process states, shouldn't we run the exit code from a sentinel?

Ambrevar commented 6 years ago

@calancha: By the way, are you still maintaining this project?

calancha commented 6 years ago

On Wed, 1 Aug 2018, Pierre Neidhardt wrote:

@calancha: By the way, are you still maintaining this project? Yes, I do. I transferred the copyright to the FSF, and the library is stored in the ELPA repository; i.e., any Emacs colleague might introduce modifications.

calancha commented 6 years ago

On Wed, 1 Aug 2018, Pierre Neidhardt wrote:

I think dired-du is a great idea, but in practice I find it very frustrating to use because each folder takes 1s to open.

I've rooted out the slow-down to the dired-du-get-recursive-dir-size-in-parallel function: notice the (sleep-for 1) call there. If I remove it, it seems to be working still, but I suspect the sleep was introduced to avoid polling.

Changing it to (sleep-for 0 10) makes for a much better experience in my opinion.

That said, instead of polling for the process states, shouldn't we run the exit code in a sentinel? Thank you very much for your report. I don't remember now why I added such `sleep-for'; it's a bit suspicious and makes me think that I felt forced to add it. I am now in vacation quite far from home. I want to look on these issues as sooner as I be back.

Ambrevar commented 6 years ago

Alright!
I'm not too familiar with ELPA: does the development happen on the emacs-elpa-diffs mailing list then? I can get access to the git repository.

calancha commented 6 years ago

On Wed, 1 Aug 2018, Pierre Neidhardt wrote:

Alright! I'm not too familiar with ELPA: does the development happen on the emacs-elpa-diffs mailing list then? You can do as you wish:

  1. It's OK send email to me.
  2. Or you can send email using the command `report-emacs-bug'. Then other colleagues would be aware of the issue, and maybe someone else can feel interested to fix the issue.

I can get access to the git repository. The ELPA repository is here: https://git.savannah.gnu.org/cgit/emacs/elpa.git

calancha commented 6 years ago

Sorry for the long time to look on this. On Wed, 1 Aug 2018, Pierre Neidhardt wrote:

Changing it to (sleep-for 0 10) makes for a much better experience in my opinion. In my machine, your suggestion improves speed ~ 25 %. I have visited the same dir 3 times with dired-du-mode enabled under following conditions:

  • Visit a directory with dired-du-mode enabled 3 times: each time kill the buffer and revisit the directory again, i.e. we are not calling `dired-revert'.
  • Use the same directory on each test.
  • Bechmark timing with: (benchmark-run (dired mydirectory))

;; Commented the line: (sleep-for 1) (0.930941392 57 0.4255784370000001) (0.926967666 52 0.45465017599999924) (0.917103985 52 0.44091792600000007) ;; Uncomment the line: (sleep-for 1) (1.082437066 3 0.027210608999999997) (1.069706278 2 0.021548911000000004) (1.073224057 2 0.018311132000000008) ;; Use: (sleep-for 0 10) (0.828726135 3 0.02860088100000001) (0.831037074 2 0.01635731500000001) (0.842839286 3 0.030896016999999998)

The first case (comment out the line) gives a bit larger time than teh 3rd case (and performs a lot more of GC).

The second case is the default one: it's consistent with your observations: significantly larger than using '(sleep-for 0 10)'.

I'm OK to change the code to the 3rd way that you suggests. If you have a better idea/patch I am glad to see it. What do you think?

Thanks

Ambrevar commented 6 years ago

I guess it's better for now, but I suggest adding a comment explaining why this is there and that we probably need to move this bit to a sentinel instead of polling.

calancha commented 6 years ago

I don't remember even yesterday lunch, but I think I can try to figure out ;-) I've added a comment. Thank you for your help!