ARROW-1896: [C++] Do not allocate memory inside CastKernel. Clean up template instantiation to not generate dead identity cast code #3642

wesm · 2019-02-14T03:42:41Z

Also resolves ARROW-4110, which has been on my list for some time.

This ended up being a huge pain.

detail::PrimitiveAllocatingUnaryKernel can now allocate memory for any kind of fixed width type.
I factored out simple bitmap propagation into detail::PropagateNulls
I moved the null count resolution code one level down into ArrayData, since there are cases where it may be set to kUnknownNullCount (e.g. after a slice) and you need to know what it is. This isn't tested but I suggest addressing this in a follow up patch

I also moved hand-maintained macro spaghetti for instantiating CastFunctors into a Python code-generation script. This might be the most controversial change in this patch, but the problem here is that we needed to exclude 1 macro case for each numeric type -- currently they were relying on NUMERIC_CASES. This means the list of generated types is slightly different for each type, lending to poor code reuse. Rather than maintaining this code by hand, it is so much simpler to generate it with a script.

Speaking of code generation, I think we should continue to invest in code generation scripts to make generating mundane C++ code for pre-compiled kernels simpler. I checked the file in but I'm not opposed to auto-generating the files as part of the CMake build -- we could do that in a follow up PR.

wesm · 2019-02-14T03:43:03Z

cpp/src/arrow/compute/kernels/cast.cc

Deleting this stuff felt sooooo good

wesm · 2019-02-14T03:43:10Z

cpp/src/arrow/compute/kernels/cast.cc

wesm · 2019-02-14T03:44:02Z

cpp/src/arrow/compute/kernels/cast.cc

This was being maintained with templates before... that was overly elaborate. If this gets too complicated, we can easily code-generate it

wesm · 2019-02-14T03:45:04Z

@emkornfield @xhochy @fsaintjacques for your comments...

emkornfield

Took a first pass through. I need to look at where the aggregate stuff landed to see if output_type makes sense there.

On code generation I think its worth experimenting with it, but we should try to have some ground rules for when it makes sense (and a CMAKE rule to regenerate automatically so generated code doesn't get out of sync with checked in code. I think I prefer seeing the generated code so I do think it should be checked in, but I think I might prefer it living in its own directory if possible).

emkornfield · 2019-02-14T04:29:26Z

cpp/src/arrow/compute/kernels/util-internal.cc

do DCHECKS support a message?

This is copy-pasta. I'll add a message

emkornfield · 2019-02-14T04:30:25Z

cpp/src/arrow/compute/kernels/util-internal.cc

why the memset?

This was in the original version, so I left it

https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/kernels/cast.cc#L1140

It probably isn't necessary, but it removing it breaks unit tests, could we file a follow-up JIRA to figure out why and remove it?

see https://issues.apache.org/jira/browse/ARROW-4572

emkornfield · 2019-02-14T04:41:33Z

cpp/src/arrow/compute/kernels/cast.cc

style nit: I know this is the convention in the code base if we pass in parameters that are going to be copied anyways, we might consider passing by value and using std::move inside the method instead. This allows for passing one time use parameters with std::move as well, which can avoid a copy. The performance benefits (especially for shared_ptrs) aren't clear

OK with me. done

emkornfield · 2019-02-14T04:42:55Z

cpp/src/arrow/compute/kernels/cast-codegen-internal.h

is it possible to indicate what script generated the file? (I know grep/directory structure would make it not too hard to find but still might be useful).

emkornfield · 2019-02-14T04:44:58Z

cpp/src/arrow/compute/kernels/cast.cc

that's only necessary in header files

emkornfield · 2019-02-14T04:55:46Z

cpp/src/arrow/compute/kernels/util-internal.h

it would be nice to add a test for this (one of the reasons I added gmock...

I didn't on the first pass because I figured I would add it as part of the TODO

emkornfield · 2019-02-14T04:56:16Z

cpp/src/arrow/compute/kernels/hash.cc

I should have pushed harder to get ValueCounts done :(

emkornfield · 2019-02-14T04:57:30Z

cpp/src/arrow/array.cc

not your change but can we use the unknown null count constant here?

emkornfield · 2019-02-14T05:01:38Z

cpp/src/arrow/compute/kernel.h

documentation?

emkornfield · 2019-02-14T05:06:47Z

cpp/src/arrow/compute/kernels/util-internal.cc

just curious why the preference for this form vs the original?

This way the memory pool doesn't have to leak. I don't have a strong preference

Neither do I just curious about the change.

wesm · 2019-02-14T05:31:27Z

I'll take care of the small comments tomorrow. I'm sort of against writing a lot more new code in this patch since the goal here is to refactor and remove dead code while leaving existing behaviors unchanged

emkornfield · 2019-02-14T07:58:55Z

cpp/src/arrow/compute/kernels/util-internal.cc

i left a bug in here that ASAN caught, we should also check that buffer_size > 0 (I can file a JIRA and submit a PR separately if you want me to handle it).

done. Can you add a unit test as a follow up?

xhochy · 2019-02-14T09:09:54Z

I can understand the need for the code generation but I still hope that someday a C++ ninja comes along and removes it again. With C++ template system and C preprocessor, I would not have expected that we need code generation.

wesm · 2019-02-14T15:16:17Z

So TensorFlow has a similar situation for casts, and they have avoided code generation:

https://github.com/tensorflow/tensorflow/blob/master/tensorflow/core/kernels/cast_op_impl.h#L36

It's easier in TensorFlow, though:

Only deals in numeric types, no date/time types, string types, or nested types to deal with
Does not appear to do zero copy optimizations (?), always produces a copy AFAICT

I view code generation as a last resort, so I don't exact us to get into the habit of it

…nside CastKernel. Use code generation to avoid instantiating CastFunctors for identity casts that are never used Change-Id: Ied8293612f46b953bab6ff1fdd737a43a65cba8f

Change-Id: I694a562066ff49863edf25eb81feba063390d0f6

Change-Id: I5180e072b4451ec575c6b9157679fa31dfba5b74

wesm · 2019-02-14T18:23:32Z

Would like to merge this once the build is passing, if there are any parting thoughts?

Change-Id: Ia083b00f529b9fc172694417e0e42cbe9b00b480

Change-Id: I5ccbff285faf8e364d939d46269ff8ae1186509c

wesm · 2019-02-15T04:55:26Z

+1. Appveyor build https://ci.appveyor.com/project/wesm/arrow/builds/22384650

wesm commented Feb 14, 2019

View reviewed changes

emkornfield reviewed Feb 14, 2019

View reviewed changes

wesm added 3 commits February 14, 2019 11:03

Refactoring around kernel memory allocation, do not allocate memory i…

7a178a4

…nside CastKernel. Use code generation to avoid instantiating CastFunctors for identity casts that are never used Change-Id: Ied8293612f46b953bab6ff1fdd737a43a65cba8f

Fix dictionary->dense conversion for Decimal128

ab534d1

Change-Id: I694a562066ff49863edf25eb81feba063390d0f6

Code review

8aeaf96

Change-Id: I5180e072b4451ec575c6b9157679fa31dfba5b74

wesm force-pushed the ARROW-1896 branch from 0e1f997 to 8aeaf96 Compare February 14, 2019 17:36

wesm added 2 commits February 14, 2019 13:04

Fix clang warning on macOS

0d3a7b3

Change-Id: Ia083b00f529b9fc172694417e0e42cbe9b00b480

Fix another clang warning

57d1084

Change-Id: I5ccbff285faf8e364d939d46269ff8ae1186509c

wesm closed this in 47ebb1a Feb 15, 2019

wesm deleted the ARROW-1896 branch February 15, 2019 04:56

asfimport mentioned this pull request Feb 15, 2019

[C++] Do not allocate memory for primitive outputs in CastKernel::Call implementation #17889

Closed

ARROW-1896: [C++] Do not allocate memory inside CastKernel. Clean up template instantiation to not generate dead identity cast code #3642

ARROW-1896: [C++] Do not allocate memory inside CastKernel. Clean up template instantiation to not generate dead identity cast code #3642

Uh oh!

Conversation

wesm commented Feb 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesm commented Feb 14, 2019

Uh oh!

emkornfield left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesm commented Feb 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xhochy commented Feb 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wesm commented Feb 14, 2019

Uh oh!

wesm commented Feb 14, 2019

Uh oh!

wesm commented Feb 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

xhochy commented Feb 14, 2019 •

edited

Loading