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

[WIP] Limit thread focus loop by time, not threads. #1519

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ryneeverett
Copy link
Contributor

Resolve #1518. The problem that search_threads_rebuild_limit tries to
solve is UI responsiveness, so the relevant thing to limit is time, not
loops.

Next steps if this approach is accepted:

  • Must: Deprecate search_threads_rebuild_limit -- Has a process been
    developed for option deprecation/removal?
  • Maybe: Add a configuration option for the timeout. Personally I don't
    think this is necessary because unlike the previous approach this is not
    relative to the user's processor and inbox size. It's always something
    that can be added if a need arises.
  • Maybe: Execute consume_pipe_until in a separate thread, sleep for the
    timeout, and then kill the thread. This would eliminate the
    wasteful clock checking every loop but I'm doubtful it's actually more
    performant.

@ryneeverett
Copy link
Contributor Author

@lenormf Any thoughts on this proposal? TLDR: I don't think unlimited search_threads_rebuild_limit is an ideal default but it seems that limiting by time is more helpful and relativizes the limitation so that a reasonable default can be chosen.

@lenormf
Copy link
Contributor

lenormf commented May 31, 2020

I don't think using time as a metric explicitly is a good thing:

  • it's always cause for arguments when setting default values (3s is a lot to some users, nothing to others etc.)
  • it performs an extra system call, which ironically is slow
  • it's redundant, since users already deal with time when setting the count limit to recover focus - they will set a value according to how fast their system can handle a given amount of messages

@ryneeverett
Copy link
Contributor Author

Thanks for the feedback.

I believe this approach better addresses the variability of preferences by removing the variability of performance as a source of disagreement. The current default blocks for several minutes for me and is presumably nothing for you. Consider this an argument. :)

As for redundancy, my proposal is to eliminate the count limit setting. Furthermore, assuming a user did want a 3 second timeout, if they had multiple machines they have to configure them differently to accommodate different performance specs.

If performance is a concern I can do some testing with different approaches but I probably wont expend the effort unless or until performance is the only concern. If I'm correct in my belief that this feature will break alot out of the box for many users, I can't see performance as a compelling reason to reject this change.

Resolve pazz#1518. The problem that search_threads_rebuild_limit tries to
solve is UI responsiveness, so the relevant thing to limit is time, not
loops.

Next steps if this approach is accepted:
- Must: Deprecate search_threads_rebuild_limit -- Has a process been
developed for option deprecation/removal?
- Maybe: Add a configuration option for the timeout. Personally I don't
think this is necessary because unlike the previous approach this is not
relative to the user's processor and inbox size. It's always something
that can be added if a need arises.
- Maybe: Execute consume_pipe_until in a separate thread, sleep for the
timeout, and then kill the thread. This would eliminate the
wasteful clock checking every loop but I'm doubtful it's actually more
performant.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

search_threads_rebuild_limit sane default
2 participants