Uh oh!
There was an error while loading. Please reload this page.
ARROW-11990: [C++][Compute] Handle errors consistently - #10098
Conversation
Turns out a much bigger change than I thought. Some hints maybe helpful to reviewer:
|
cyb70289
commented
Apr 20, 2021
@github-actions crossbow submit -g nightly |
Revision: b7229010b5413113a6274f7ad6d6d9ad900ff2e8 Submitted crossbow builds: ursacomputing/crossbow @ actions-336 |
Benchmark diff of all computer kernels, skylake, clang-9. Removed tests with less than 10% deviation. |
cyb70289
commented
Apr 20, 2021
MacOS CI error is due to LLVM-12 update, https://issues.apache.org/jira/browse/ARROW-12467 |
pitrou
commented
Apr 20, 2021
Here are the changes (only with |
pitrou
commented
Apr 20, 2021
I'm in support of this. Performance improvements notwithstanding, the main benefits IMHO are:
|
nealrichardson
commented
Apr 20, 2021
Out of curiosity, why does this improve performance? |
pitrou
commented
Apr 20, 2021
@nealrichardson Not sure, probably it makes things easier for the compiler. |
pitrou
commented
Apr 20, 2021
westonpace
commented
Apr 20, 2021
@nealrichardson In the realm of wild guesses I would investigate before taking much stock in, a change like this... ...changes from setting a variable on a class (that variable could then potentially be read in a lot of different places) to an out parameter (which can only be seen by the caller). By reducing the visible scope of the variable you are writing too you increase the chance the compiler decides that no one else needs to see the change and it can keep it in a register instead of writing it out to RAM somewhere. |
cyb70289
commented
Apr 22, 2021
Rebased and fixed merge conflicts. |
cyb70289
commented
Apr 22, 2021
MinGW32 python test timeout looks a spurious issue happens occasionally. |
bkietz
left a comment
There was a problem hiding this comment.
Thanks a lot for this cleanup, looks great!
Two minor nits:
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Arrow handles errors by returning Status/Result. But in compute kernels, errors are populated in KernelContext.status. This is not consistent, and updating KernelContext.status is not thread safe. This patch removes KernelContext.status and returns kernel errors as Status/Result.
cyb70289
commented
Apr 27, 2021
RTools CI failure looks not related. @nealrichardson |
pitrou
commented
Apr 27, 2021
Really a nice improvement, thank you! |
pitrou
commented
Apr 27, 2021
I do think the RTools failures are unrelated (I see them on other PRs), so will merge. |
nealrichardson
commented
Apr 27, 2021
FTR the R failure is This is because the 4.0.0 release just his CRAN, and apparently we have not bumped the version numbers post-release on master yet. @kszucs? |
Arrow handles errors by returning Status/Result. But in compute kernels,
errors are populated in KernelContext.status. This is not consistent,
and updating KernelContext.status is not thread safe.
This patch removes KernelContext.status and returns kernel errors as
Status/Result.
See big performance improvement for arithmetic kernels, especially the
checked version (up to 4x).
Also see ~50% drops from some filter kernels. Will investigate deeper
as follow up task.