refactor: dead c++ elimination #2504
Conversation
Codecov Report
Additional details and impacted files
|
| inline struct Error | ||
| failure_pass_through( | ||
| const char* str, | ||
| int64_t identity, | ||
| int64_t attempt, | ||
| const char* filename) { | ||
| struct Error out; | ||
| out.str = str; | ||
| out.filename = filename; | ||
| out.identity = identity; | ||
| out.attempt = attempt; | ||
| out.pass_through = true; |
There was a problem hiding this comment.
I'm removing this because, seeing as none of our kernels use it, it's unlikely to be useful. I assume it's legacy from the C++ era?
There was a problem hiding this comment.
Yes, that's right. It looks like we're letting pass_through be undefined now, which is a little nerve-wracking. If we're going to change the Error struct, we'll have to carefully check that nothing relies on its exact memory layout (in the Numba code). Meanwhile, if you want to set it to always false, just so that it's not undefined, that sounds safer.
jpivarski
added
the
pr-on-hold
|
On hold until after an |
agoose77
removed
the
pr-on-hold
| inline struct Error | ||
| failure_pass_through( | ||
| const char* str, | ||
| int64_t identity, | ||
| int64_t attempt, | ||
| const char* filename) { | ||
| struct Error out; | ||
| out.str = str; | ||
| out.filename = filename; | ||
| out.identity = identity; | ||
| out.attempt = attempt; | ||
| out.pass_through = true; |
There was a problem hiding this comment.
Yes, that's right. It looks like we're letting pass_through be undefined now, which is a little nerve-wracking. If we're going to change the Error struct, we'll have to carefully check that nothing relies on its exact memory layout (in the Numba code). Meanwhile, if you want to set it to always false, just so that it's not undefined, that sounds safer.
| const char* filename; | ||
| int64_t identity; | ||
| int64_t attempt; | ||
| bool pass_through; | ||
| }; |
There was a problem hiding this comment.
I spoke incorrectly: you are actually removing the pass_through field from the Error struct.
I looked through all the files in https://github.com/scikit-hep/awkward/tree/main/src/awkward/_connect/numba and I don't see anything that references the Error struct, so I guess all of that has been removed. (Earlier, some Numba code was calling these kernels directly; that was part of the design decision to make them purely extern "C". Calling external function pointers from Numba is slow—slower than would be expected of just not being able to optimize around it, for some reason—and so we gave that up. There are apparently no further traces of it. But if there were any, if Numba called kernels and expected this Error struct in return, it would have to make an assumption about its memory layout, and we wouldn't want those assumptions to get out of sync.)
Short story: it looks to me like it's okay to change the Error struct, so full speed ahead.
| ("filename", c_char_p), | ||
| ("id", c_int64), | ||
| ("attempt", c_int64), | ||
| ("pass_through", c_bool), | ||
| ] |
There was a problem hiding this comment.
This is another thing that independently makes assumptions about the Error struct, but you've fixed it.
Alas no, I erred in favour of the biological computer living upstairs ;) |
Now that the C++ split has settled, I did another pass for any unused code and found the following. It's unfortunate that this comes after a C++ release, but we don't need to merge it soon.
The motivation for removing this code is that most of it is untested.