FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add throttling support for callbacks by srajko · Pull Request #932 · nodegit/nodegit · GitHub

Repository navigation

Add throttling support for callbacks - #932

Merged
johnhaley81 merged 6 commits into
nodegit:masterfrom
srajko:callback-throttle
Mar 3, 2016
Merged

johnhaley81 merged 6 commits into
nodegit:masterfrom
srajko:callback-throttle

Conversation

srajko commented Mar 1, 2016

Copy link
Copy Markdown
Collaborator

No description provided.

Copy link
Copy Markdown
Collaborator

This will fix #919

srajko force-pushed the callback-throttle branch from be11a91 to 6aa0398 Compare March 2, 2016 17:19

Copy link
Copy Markdown
Collaborator

@srajko can you add a test to confirm that the callback is indeed being throttled? It could be another clone test.

srajko force-pushed the callback-throttle branch 2 times, most recently from 8b961c7 to 8672782 Compare March 3, 2016 17:19

srajko commented Mar 3, 2016

Copy link
Copy Markdown
Collaborator Author

The changes are as follows:

  • Progress callbacks (git_checkout_progress_cb, git_diff_file_cb, git_stash_apply_progress_cb, git_transfer_progress_cb) now have default throttling of ~1 callback per 100ms
  • All callbacks provided through configuration objects can now be throttled - instead of passing a function, pass an object { callback, throttle } where throttle is the throttle period (~1 callback per throttle period, in ms). Passing throttle: 0 disables throttling.

See commit descriptions for a rundown.

srajko force-pushed the callback-throttle branch from 8672782 to 963d0f2 Compare March 3, 2016 17:36
srajko added 6 commits March 3, 2016 12:13
just refactoring, eliminating duplicated code
 I originally tried doing the throttling in the baton but that ended up being a mistake. This moved the default result to the baton so the baton could use it. I don't think the change is necessary for the final version of this PR, but I still like moving things out of combyne-templated files so I kept it.
just refactoring, leverages the previously unused CallbackWrapper to set us up for bundling throttling state with the javascript callback.
default settings for progress callbacks.
srajko force-pushed the callback-throttle branch from 963d0f2 to b6215ac Compare March 3, 2016 19:15
srajko changed the title [WIP] Add throttling support for callbacks Add throttling support for callbacks Mar 3, 2016

Copy link
Copy Markdown
Collaborator

👍 nice job! Thanks @srajko

bool done;
};

template<typename ResultT>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

💯

johnhaley81 added a commit that referenced this pull request Mar 3, 2016
Add throttling support for callbacks
johnhaley81 merged commit 9981139 into nodegit:master Mar 3, 2016
johnhaley81 deleted the callback-throttle branch March 3, 2016 21:10
johnhaley81 added a commit that referenced this pull request Mar 7, 2016
A non-trivial number of segfaults are seeing this in production. Pulling this back for right now until a later release where we can fix them.
johnhaley81 added a commit that referenced this pull request Mar 7, 2016
Revert "Merge pull request #932 from srajko/callback-throttle"
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

2 participants


Back | FazBrowse Home | New Git URL