-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add cancellation signal checkpoints in FastTree. #3028
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3028 +/- ##
==========================================
+ Coverage 72.41% 72.49% +0.07%
==========================================
Files 803 804 +1
Lines 143851 144086 +235
Branches 16173 16179 +6
==========================================
+ Hits 104171 104452 +281
+ Misses 35258 35221 -37
+ Partials 4422 4413 -9
|
Hi @codemzs this looks pretty good I think. While doing this check during the histogram calcaulation is good, one area that can take a significant amount of time that is not covered at all by this change as far as I see is the dataset preparation -- your analysis from the issue might not have captured it, but it does on many datasets take a significant amount of time, and happening, as it does, at the start of the operation it is one of the most likely places where a cancellation might actually occur. What do you think? |
@TomFinley You are right. My analysis was done without disk transpose so I missed the transpose function but now I have added checkpoints there. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good, thank you @codemzs !!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixes #3027
Please read the issue before reviewing this PR.