You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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
Cython's gcc branch prediction directive make sag unnecessarily fight for the GIL at a high frequency in case an error needs to be raised. This severely impacts the performance of parallel sag calls using a joblib"threading" backend.
I propose to handle numerical error "C-style", by propagating return codes and breaking out of for-loops. An error is raised once and for all after sag, in sag_solver.
Note that I need to manually break out of for-loops and not simply return -1 after the check, as returning requires the GIL, so the same problem would happen.
@jeremiedbb followed the issue during last sprint.
The reason will be displayed to describe this comment to others. Learn more.
This is a really weird performance bug. Have you tried to re-run your benchmark with this PR to confirm that it fixes the performance issue observed with the threading backend?
Can you try to compile this extension without branch prediction to confirm that the GIL acquisition is caused by this as suggested in #13316 (comment)? That sounds really weird to me.
This is a really weird performance bug. Have you tried to re-run your benchmark with this PR to confirm that it fixes the performance issue observed with the threading backend?
Can you try to compile this extension without branch prediction to confirm that the GIL acquisition is caused by this as suggested in #13316 (comment)? That sounds really weird to me.
This is strange indeed but if you comment the
ifsomething_not_finite:
withgil:
raiseerror
bloks, the issue disappears, although the condition is never met (because no error is raised)
@ogrisel I tried disabling Cython.Compiler.Options.gcc_branch_hints using current master but that did not improve performance.
However, not trying to aquire the gil in such setting is not unusual: in sgd_fast.pyx, the same "trick" is done:
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #13316
Cython's gcc branch prediction directive make
sag
unnecessarily fight for the GIL at a high frequency in case an error needs to be raised. This severely impacts the performance of parallelsag
calls using ajoblib
"threading"
backend.I propose to handle numerical error "C-style", by propagating return codes and breaking out of for-loops. An error is raised once and for all after
sag
, insag_solver
.Note that I need to manually break out of for-loops and not simply return -1 after the check, as returning requires the GIL, so the same problem would happen.
@jeremiedbb followed the issue during last sprint.