ARROW-6458: [Java] Remove value boxing/unboxing for ApproxEqualsVisitor

[liyafan82]

As discussed in #5195 (comment), there are some problems with the current ways of comparing floating point vectors, we solve them in this PR:

1. there are if statements/duplicated members in ApproxEqualsVisitor, making the code redundant and less clear.
2. the comparion of float4 and float8 are based on wrapped objects Float and Double, which may have performance penalty.

[emkornfield]

@liyafan82 I think this change should probably be broken into two different changes. One that avoid boxing/unboxing and a second one that consolidates code. The code consolidation has the potential to actually hurt performance. As part of the boxing/unboxing we should also introduce a performance test to verify it actually improves performance (which can then be used to measure the impact of the code consolidation behind the interface).

[liyafan82]

@liyafan82 I think this change should probably be broken into two different changes. One that avoid boxing/unboxing and a second one that consolidates code. The code consolidation has the potential to actually hurt performance. As part of the boxing/unboxing we should also introduce a performance test to verify it actually improves performance (which can then be used to measure the impact of the code consolidation behind the interface).

It is a good suggestion to split into two issues for the two problems. I will revise the code and provide the performance benchmark accordingly.

[liyafan82]

@emkornfield I have split the issue into two, according to your suggestion.
This is the first part, to remove boxing/unboxing of floating point values.

Performance benchmark show that there is a performance improvement of over 10%:

Before:
Float8Benchmarks.approxEqualsBenchmark avgt 5 7.161 ± 0.026 us/op

After
Float8Benchmarks.approxEqualsBenchmark avgt 5 6.355 ± 0.004 us/op

[emkornfield]

please add float4 vectors as part of this benchmark.

[liyafan82]

Sure. I will add benchmarks for float4 to class Float4Benchmarks.

[emkornfield]

Please consolidate into 1 floatbenchmarks class and a single benchmark. (reasoning behind this: http://insightfullogic.com/2014/May/12/fast-and-megamorphic-what-influences-method-invoca/)

[liyafan82]

Sure. Thanks for the article. We have also found some problems with megamorphic methods in the current code base. Will improve them later.

[emkornfield]

For things not on the hot path it probably isn't worth changing. The only reason I'm bringing it up here is because the code seemed to already written to avoid those calls. If there are a lot of places you intend to fix then having a discussion on the mailing list would be a good idea.

[liyafan82]

Sure. Sounds reasonable.

[emkornfield]

I think this change is too much, I think the right thing to do is introduce an interface is to change DiffFunction into two interface.

DiffFunctionFloat {
bool approxEquals(float v1, float v2);
}

DiffFunctionDouble {
bool approxEquals(double v1, double v2);
}

and

[liyafan82]

I see. Will revise accordingly.

[liyafan82]

@emkornfield
I have revised the code to make the changes smaller.
And I have merged the benchmarks to a single class and a single benchmark, according to your suggestion.

The benchmark results are as follows:
Before:
FloatingPointBenchmarks.approxEqualsBenchmark avgt 5 14.480 ± 0.023 us/op

After:
FloatingPointBenchmarks.approxEqualsBenchmark avgt 5 13.517 ± 0.041 us/op

By removing boxing/unboxing, we have a performance improvement of about 6.7%

[emkornfield]

See comments.

[codecov-io]

Codecov Report

Merging #5304 into master will increase coverage by 0.98%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5304      +/-   ##
==========================================
+ Coverage   88.59%   89.57%   +0.98%     
==========================================
  Files         950      702     -248     
  Lines      126185   107156   -19029     
  Branches     1495        0    -1495     
==========================================
- Hits       111791    95990   -15801     
+ Misses      14029    11166    -2863     
+ Partials      365        0     -365

Impacted Files	Coverage Δ
cpp/src/arrow/filesystem/s3_internal.h	90.74% <0%> (-3.71%)	⬇️
cpp/src/arrow/json/converter.cc	90.05% <0%> (-1.76%)	⬇️
cpp/src/arrow/json/chunked_builder.cc	79.91% <0%> (-1.68%)	⬇️
cpp/src/plasma/thirdparty/ae/ae.c	70.75% <0%> (-0.95%)	⬇️
python/pyarrow/tests/test_parquet.py	96.09% <0%> (-0.12%)	⬇️
r/src/recordbatch.cpp
go/arrow/math/uint64_amd64.go
r/R/list.R
r/src/symbols.cpp
r/src/array_to_vector.cpp
... and 244 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 abc7860...907c17d. Read the comment docs.

[emkornfield]

+1, thank you.