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

OPT: Rate limit result suppression message update #5060

Merged
merged 4 commits into from Oct 22, 2020

Conversation

mih
Copy link
Member

@mih mih commented Oct 21, 2020

Updating it for each result is the cause of the slow-down reported in
#4868 (comment)

With this change we only update at max 2Hz. For the original scenario
this results in a ~30% speed-up:

datalad subdatasets -d .  4.30s user 1.99s system 101% cpu 6.225 total
datalad subdatasets -d .  6.33s user 2.27s system 100% cpu 8.538 total

Updating it for each result is the cause of the slow-down reported in
datalad#4868 (comment)

With this change we only update at max 2Hz. For the original scenario
this results in a ~30% speed-up:

```
datalad subdatasets -d .  4.30s user 1.99s system 101% cpu 6.225 total
datalad subdatasets -d .  6.33s user 2.27s system 100% cpu 8.538 total
```
@mih mih added performance Improve performance of an existing feature UX user experience labels Oct 21, 2020
@mih mih requested a review from yarikoptic October 21, 2020 14:47
@@ -514,16 +515,24 @@ def default_result_renderer(res):
if res.get('message', None) else ''))


def _display_suppressed_message(nsimilar, ndisplayed, final=False):
def _display_suppressed_message(nsimilar, ndisplayed, last_ts, final=False):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This direction sounds good to me, but I guess that the rate-limiting shouldn't be in effect when final=True?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Adjusted, thx.

@codecov
Copy link

codecov bot commented Oct 21, 2020

Codecov Report

Merging #5060 into maint will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##            maint    #5060   +/-   ##
=======================================
  Coverage   89.96%   89.96%           
=======================================
  Files         292      292           
  Lines       40863    40875   +12     
=======================================
+ Hits        36761    36774   +13     
+ Misses       4102     4101    -1     
Impacted Files Coverage Δ
datalad/interface/tests/test_utils.py 100.00% <100.00%> (ø)
datalad/interface/utils.py 94.81% <100.00%> (+0.51%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39939f1...9fb900d. Read the comment docs.

mih and others added 2 commits October 22, 2020 09:21
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
Even if it is triggered faster than the rate limit.
@mih mih merged commit 9975442 into datalad:maint Oct 22, 2020
1 of 2 checks passed
@mih mih deleted the opt-resultsuppression branch October 22, 2020 09:11
@yarikoptic yarikoptic added this to the 0.13.5 milestone Dec 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Improve performance of an existing feature UX user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants