Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Add throttling support for callbacks#932

Merged
johnhaley81 merged 6 commits intonodegit:masternodegit/nodegit:masterfrom
srajko:callback-throttlesrajko/nodegit:callback-throttleCopy head branch name to clipboard
Mar 3, 2016
Merged

Add throttling support for callbacks#932
johnhaley81 merged 6 commits intonodegit:masternodegit/nodegit:masterfrom
srajko:callback-throttlesrajko/nodegit:callback-throttleCopy head branch name to clipboard

Conversation

@srajko
Copy link
Collaborator

@srajko srajko commented Mar 1, 2016

No description provided.

@johnhaley81
Copy link
Collaborator

This will fix #919

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

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

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

srajko commented Mar 3, 2016

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 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 srajko force-pushed the callback-throttle branch from 963d0f2 to b6215ac Compare March 3, 2016 19:15
@srajko srajko changed the title [WIP] Add throttling support for callbacks Add throttling support for callbacks Mar 3, 2016
@johnhaley81
Copy link
Collaborator

👍 nice job! Thanks @srajko

bool done;
};

template<typename ResultT>
Copy link
Collaborator

Choose a reason for hiding this comment

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

💯

johnhaley81 added a commit that referenced this pull request Mar 3, 2016
Add throttling support for callbacks
@johnhaley81 johnhaley81 merged commit 9981139 into nodegit:master Mar 3, 2016
@johnhaley81 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"
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

Morty Proxy This is a proxified and sanitized view of the page, visit original site.