Reduce redundant code in CUDF JNI #10019

mythrocks · 2022-01-11T18:24:53Z

This commit cleans up some of the redundancy in CUDF JNI native code, and modifies some of the unique_ptr handling:

Improvements

Added utility function to convert from native_jpointerArray<column_view> to vector<column_view>. Removed redundant code in stringConcat functions.
Added utility to convert from unique_ptr<column> to jlong. This eliminates variables from nearly all the JNI functions.
Added utility functions for extracting std::vector from native_jArray classes. This reduces raw loops in TableJni.cpp.
Eliminated unnecessary use of unique_ptr in locations where they are immediately release()d. (E.g. ColumnVectorJni::getNativeColumnView().)
Removed unnecessary std::move() calls.
Removed unnecessary reinterpret_cast (E.g. in copyColumnViewToCV().)
typedef -> using.

Also, remove repeated code, fix unique_ptr usage.

1. Memory leak in hashing JNI code. 2. Removed convoluted/unnecessary uses of std::unique_ptr. 3. Removed unnecessary uses of std::move. 4. Moved repeated std::transform() for column/column_view conversions to utility function. 5. Removed unnecessary variables, shortened jlong casting. Yet to address memory leak from bitwiseMergeAndSetValidity.

java/src/main/native/include/jni_utils.hpp

java/src/main/native/src/ColumnViewJni.cpp

Plus, review comments: 1. Renamed `to_jlong(unique_ptr)` to `release_as_jlong(unique_ptr)`. 2. Copyright dates. Signed-off-by: MithunR <[email protected]>

…rray-fix

codecov · 2022-01-12T01:19:46Z

Codecov Report

Merging #10019 (5d3582d) into branch-22.04 (e24fa8f) will increase coverage by 0.04%.
The diff coverage is 0.00%.

@@               Coverage Diff                @@
##           branch-22.04   #10019      +/-   ##
================================================
+ Coverage         10.37%   10.42%   +0.04%     
================================================
  Files               119      119              
  Lines             20149    20609     +460     
================================================
+ Hits               2091     2148      +57     
- Misses            18058    18461     +403

Impacted Files	Coverage Δ
python/cudf/cudf/__init__.py	`0.00% <ø> (ø)`
python/cudf/cudf/_fuzz_testing/io.py	`0.00% <0.00%> (ø)`
python/cudf/cudf/_fuzz_testing/orc.py	`0.00% <ø> (ø)`
python/cudf/cudf/_fuzz_testing/parquet.py	`0.00% <ø> (ø)`
python/cudf/cudf/_fuzz_testing/utils.py	`0.00% <0.00%> (ø)`
python/cudf/cudf/api/types.py	`0.00% <0.00%> (ø)`
python/cudf/cudf/core/_base_index.py	`0.00% <0.00%> (ø)`
python/cudf/cudf/core/column/__init__.py	`0.00% <ø> (ø)`
python/cudf/cudf/core/column/column.py	`0.00% <0.00%> (ø)`
python/cudf/cudf/core/column/decimal.py	`0.00% <0.00%> (ø)`
... and 81 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52a61b7...5d3582d. Read the comment docs.

mythrocks · 2022-01-12T18:39:52Z

Though it could stand to be improved, I think I'll leave convert_table_for_return() intact for this iteration.

Converted native_jbooleanArray into a separate class. Added utilities for extracting std::vectors, transform_if_else(), etc. Signed-off-by: MithunR <[email protected]>

revans2

Looking really good

java/src/main/native/include/jni_utils.hpp

java/src/main/native/src/ColumnVectorJni.cpp

Removed explicit loops in map initialization. More to_jlong() fixes. Signed-off-by: MithunR <[email protected]>

…rray-fix

mythrocks · 2022-01-20T23:05:45Z

I've rebased this on branch-22.04. Further modifications (E.g. to convert_table_for_return()) can be deferred to a follow-up.
@jlowe: might I bother you to have another look?

mythrocks · 2022-01-21T18:08:49Z

Rerun tests

jlowe · 2022-01-21T18:20:20Z

I just noticed that there are a number of reinterpret_cast<jlong> occurrences still in TableJni.cpp, as well as a number remaining elsewhere in other files, including ones that are of the form reinterpret_cast<jlong>(x.release()). Were these left intentionally?

mythrocks · 2022-01-24T18:16:59Z

I just noticed that there are a number of reinterpret_cast occurrences still in TableJni.cpp...

The ones I left alone (e.g. the join code you added most recently) seemed fine to my eye, at the time. I should probably make another pass and confirm. Perhaps reconsider avoiding changes to convert_table_for_return(). The reinterpret_cast<jlong>(x.release()) sounds avoidable.

…rray-fix

mythrocks · 2022-01-24T23:34:38Z

there are a number of reinterpret_cast<jlong> occurrences

These occurrences have been removed from ColumnVectorJni.cpp, ColumnViewJni.cpp, and TableJni.cpp.

…rray-fix

mythrocks · 2022-01-25T18:42:34Z

Rebased to latest, including #10040. Waiting on CI before merging.

mythrocks · 2022-01-25T21:36:18Z

@gpucibot merge

mythrocks · 2022-01-25T21:37:25Z

Thanks for the reviews, @revans2, @jlowe. This is now merged.

mythrocks added 3 commits January 7, 2022 16:47

Streamline native_jpointerArray use.

41b2fc1

Also, remove repeated code, fix unique_ptr usage.

Formatting changes.

082c319

mythrocks requested a review from a team as a code owner January 11, 2022 18:24

mythrocks self-assigned this Jan 11, 2022

github-actions bot added the Java Affects Java cuDF API. label Jan 11, 2022

mythrocks marked this pull request as draft January 11, 2022 18:25

mythrocks changed the title ~~Reduce redundant code in CUDF JNI~~ Reduce redundant code, fix leaks in CUDF JNI Jan 11, 2022

mythrocks changed the title ~~Reduce redundant code, fix leaks in CUDF JNI~~ Fix memory leaks, reduce redundant code in CUDF JNI Jan 11, 2022

jlowe reviewed Jan 11, 2022

View reviewed changes

Further fixes in ColumnViewJni.cpp.

a1a6401

Plus, review comments: 1. Renamed `to_jlong(unique_ptr)` to `release_as_jlong(unique_ptr)`. 2. Copyright dates. Signed-off-by: MithunR <[email protected]>

mythrocks added bug Something isn't working code quality non-breaking Non-breaking change labels Jan 11, 2022

Merge remote-tracking branch 'origin/branch-22.02' into jni-jpointera…

2a48a43

…rray-fix

mythrocks requested a review from jlowe January 12, 2022 18:38

First stab at TableJni.cpp.

2b8be82

Converted native_jbooleanArray into a separate class. Added utilities for extracting std::vectors, transform_if_else(), etc. Signed-off-by: MithunR <[email protected]>

revans2 reviewed Jan 12, 2022

View reviewed changes

java/src/main/native/include/jni_utils.hpp Outdated Show resolved Hide resolved

jlowe reviewed Jan 12, 2022

View reviewed changes

java/src/main/native/src/ColumnVectorJni.cpp Outdated Show resolved Hide resolved

mythrocks added 2 commits January 12, 2022 16:31

More changes to TableJni.cpp:

1495445

Removed explicit loops in map initialization. More to_jlong() fixes. Signed-off-by: MithunR <[email protected]>

Merge remote-tracking branch 'origin/branch-22.02' into jni-jpointera…

b42291a

…rray-fix

mythrocks changed the title ~~Fix memory leaks, reduce redundant code in CUDF JNI~~ Reduce redundant code in CUDF JNI Jan 13, 2022

mythrocks added improvement Improvement / enhancement to an existing function tech debt and removed bug Something isn't working labels Jan 13, 2022

github-actions bot removed conda CMake CMake build issue labels Jan 13, 2022

mythrocks marked this pull request as ready for review January 13, 2022 23:23

mythrocks requested a review from a team as a code owner January 13, 2022 23:23

mythrocks requested review from harrism and jrhemstad and removed request for harrism and jrhemstad January 13, 2022 23:23

revans2 approved these changes Jan 14, 2022

View reviewed changes

Merge remote-tracking branch 'origin/branch-22.04' into jni-jpointera…

abcca8a

…rray-fix

github-actions bot removed the libcudf Affects libcudf (C++/CUDA) code. label Jan 20, 2022

mythrocks requested a review from jlowe January 20, 2022 23:03

jlowe approved these changes Jan 20, 2022

View reviewed changes

mythrocks removed the request for review from a team January 21, 2022 18:08

mythrocks added 2 commits January 24, 2022 10:20

Merge remote-tracking branch 'origin/branch-22.04' into jni-jpointera…

e6a3ffd

…rray-fix

Removed explict reinterpret_cast<jlong(ptr.release())

5da8024

Merge remote-tracking branch 'origin/branch-22.04' into jni-jpointera…

5d3582d

…rray-fix

rapids-bot bot merged commit 22daaea into rapidsai:branch-22.04 Jan 25, 2022

mythrocks deleted the jni-jpointerarray-fix branch January 25, 2022 21:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce redundant code in CUDF JNI #10019

Reduce redundant code in CUDF JNI #10019

mythrocks commented Jan 11, 2022 •

edited

Loading

codecov bot commented Jan 12, 2022 •

edited

Loading

mythrocks commented Jan 12, 2022

revans2 left a comment

mythrocks commented Jan 20, 2022 •

edited

Loading

mythrocks commented Jan 21, 2022

jlowe commented Jan 21, 2022

mythrocks commented Jan 24, 2022 •

edited

Loading

mythrocks commented Jan 24, 2022

mythrocks commented Jan 25, 2022

mythrocks commented Jan 25, 2022

mythrocks commented Jan 25, 2022

Reduce redundant code in CUDF JNI #10019

Reduce redundant code in CUDF JNI #10019

Conversation

mythrocks commented Jan 11, 2022 • edited Loading

Improvements

codecov bot commented Jan 12, 2022 • edited Loading

Codecov Report

mythrocks commented Jan 12, 2022

revans2 left a comment

Choose a reason for hiding this comment

mythrocks commented Jan 20, 2022 • edited Loading

mythrocks commented Jan 21, 2022

jlowe commented Jan 21, 2022

mythrocks commented Jan 24, 2022 • edited Loading

mythrocks commented Jan 24, 2022

mythrocks commented Jan 25, 2022

mythrocks commented Jan 25, 2022

mythrocks commented Jan 25, 2022

mythrocks commented Jan 11, 2022 •

edited

Loading

codecov bot commented Jan 12, 2022 •

edited

Loading

mythrocks commented Jan 20, 2022 •

edited

Loading

mythrocks commented Jan 24, 2022 •

edited

Loading