ARROW-2822: [C++] Zero padding bytes in PoolBuffer #2239

alendit · 2018-07-10T12:46:45Z

codecov-io · 2018-07-10T15:04:51Z

Codecov Report

Merging #2239 into master will increase coverage by 2.4%.
The diff coverage is 89.45%.

@@            Coverage Diff            @@
##           master    #2239     +/-   ##
=========================================
+ Coverage   84.25%   86.66%   +2.4%     
=========================================
  Files         290      236     -54     
  Lines       44305    41724   -2581     
=========================================
- Hits        37330    36160   -1170     
+ Misses       6944     5564   -1380     
+ Partials       31        0     -31

Impacted Files	Coverage Δ
cpp/src/arrow/util/io-util.h	`100% <ø> (ø)`	⬆️
cpp/src/arrow/adapters/orc/adapter.cc	`0.27% <0%> (ø)`	⬆️
cpp/src/arrow/io/hdfs.cc	`0.33% <0%> (-0.01%)`	⬇️
cpp/src/arrow/test-util.h	`74.61% <100%> (-1.36%)`	⬇️
cpp/src/arrow/util/lazy.h	`100% <100%> (ø)`
cpp/src/arrow/buffer-test.cc	`100% <100%> (ø)`	⬆️
cpp/src/arrow/io/memory.cc	`86.3% <100%> (+0.09%)`	⬆️
cpp/src/arrow/util/hash.cc	`100% <100%> (ø)`	⬆️
cpp/src/arrow/array-test.cc	`100% <100%> (ø)`	⬆️
cpp/src/arrow/python/numpy_to_arrow.cc	`89.06% <100%> (ø)`	⬆️
... and 72 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 f8cd36a...95bae66. Read the comment docs.

pitrou

Thanks for doing this! On the principle this looks sane. A couple comments below.

pitrou · 2018-07-12T09:50:21Z

cpp/src/arrow/builder.cc

Is this desirable? If an error occurs while building an array, it is normal not to finish the builder.

Yes, as mentioned in JIRA, it throws a lot of false positive. How about I set the warning level to ARROW_DEBUG, so it doesn't get printed by default, but still can be used to find problems, if the need arises?

I don't think there should be a warning at this point at all. What could emit a warning are the various builder methods that allow to bypass the Finish call (for example ArrayBuilder::null_bitmap).

How about hiding this behind a flag like ARROW_LOG_UNFINISHED_BUILDERS. Having this output in debug builds always seems like too much to me

I want to rework it and move the warning to null_bitmap and data methods. It seems to be doable, but I'd need some more time.

We can still then move it behind a special compile flag.

pitrou · 2018-07-12T09:51:48Z

cpp/src/arrow/builder.cc

Note null buffers are problematic, see PR #2243.

I just left a review + comments there. I think we should try to merge this before merging #2243 so we can decide what to do there

pitrou · 2018-07-12T09:53:49Z

cpp/src/arrow/buffer.h

Not a native English speaker, but "between padding" looks grammatically incorrect.

"in" instead of "between" sounds fine

Yeah, comment editing fail, I'll fix it.

wesm · 2018-07-12T16:40:55Z

cpp/src/arrow/buffer.h

"in" instead of "between" sounds fine

wesm · 2018-07-12T16:44:56Z

cpp/src/arrow/builder.cc

How about hiding this behind a flag like ARROW_LOG_UNFINISHED_BUILDERS. Having this output in debug builds always seems like too much to me

wesm · 2018-07-12T16:46:28Z

cpp/src/arrow/builder.cc

What's the rationale for this change? All values in NullBuilder are null

Oh, then I've misunderstood it's purpose. I thought it should be used as in python_to_arrow.cc.

I'll revert it and add some unit cases, you could look at.

wesm · 2018-07-12T16:47:03Z

cpp/src/arrow/builder.cc

Ditto re: decision above

wesm · 2018-07-12T16:48:08Z

cpp/src/arrow/builder.cc

Is setting unfinished here needed?

Since unfinished signals that there were values appended after the last FinishInternal call, I would say yes.

wesm · 2018-07-12T16:48:20Z

cpp/src/arrow/builder.cc

Curious why this is needed

There are nulls appended to the values_builder_, so it could resized and contain uninitialized padding, if we never call finish on it.

wesm · 2018-07-12T16:49:06Z

cpp/src/arrow/builder.h

Seems like this should only be set when the builder is initialized

That was what I've tried at first. Then I hit the problem that sometimes the builders are initialized but never appended to and never finished, i.e. in DictionaryBuilder. So I changed the logic to "set to false on every append".

I'll see if I can change it back after the rework I've mentioned before.

wesm · 2018-07-12T16:50:30Z

cpp/src/arrow/io/file.cc

It seems error prone to have to remember to zero the padding. What would be the performance consequences (if any, realistically) of zeroing in Resize?

Some things may be repeatedly resized if you don't know the exact size up front (e.g. in BinaryBuilder).

Right, in the event that you shrank the buffer in BinaryBuilder when Finish is called, you would want to zero the padding there, too

I guess if you shrink the memory then the memory you padded before goes unused

The worst case is when you Reserve a huge amount of memory and then gradually Resize it, as the new elements come in. You would expect these operations to be virtually free while in reality you are zeroing everything between size and capacity on every call.

Got it. OK, let's leave things as they are, then

wesm · 2018-07-12T16:51:39Z

cpp/src/arrow/python/python_to_arrow.cc

the way that NullBuilder is being used in this module doesn't look right; probably related to the NullBuilder changes above cc @pcmoritz. It looks like a BooleanBuilder should be used instead

I reverted the NullBuilder changes and switched nones_ to BooleanBuilder.

@pcmoritz could you check if 6753dcc is fine?

alendit · 2018-07-12T18:01:04Z

Hi guys. I'll try to incorporate the changes and rework the warning logic to be more precise in the comming days (hopefully tomorrow).

alendit · 2018-07-13T12:57:44Z

Ok, so there was a possibility I totally haven't considered before: we can just remove the data() and null_bitmap from the builders. Amazingly, it doesn't break much besides couple of tests and the DictionaryBuilder. I adjusted DictionaryBuilder and it seems to work like a charm. No is_finished_ shenanigans and no hacks to filter out false positives.

Does anyone know, if some 3rd party code relies on these methods? I would argue, they should be considered implementation detail, but we might want to put a deprecation warning before removing them alltogether.

pitrou · 2018-07-13T15:02:15Z

Does anyone know, if some 3rd party code relies on these methods? I would argue, they should be considered implementation detail, but we might want to put a deprecation warning before removing them alltogether.

I would deprecate them first indeed.

alendit · 2018-07-16T07:34:15Z

Rebased on the master.

pitrou

Looks like the deprecation may be problematic after all. Sorry for all the back-and-forth!

pitrou · 2018-07-16T16:18:40Z

cpp/src/arrow/adapters/orc/adapter.cc

Hmm... I think it's a bit problematic that we seem to be introducing a second data copy here (first from source to converted, then from converted to the builder). Can we avoid the second copy without using builder->data()? Otherwise we may have to undeprecate the latter.

I was also confused to see a stack allocation here and above. Is it not possible to use source in AppendValues?

Note we don't have any unit tests for this code path so have to be extra careful

pitrou · 2018-07-16T16:20:09Z

cpp/src/arrow/buffer.h

I think "fixed-size" is the correct spelling. Perhaps a native English speaker can confirm?

I googled a bit. "fixed size" seems more common, and reads OK to me

pitrou · 2018-07-16T16:20:30Z

cpp/src/arrow/buffer.h

I think the article here ("a resizeable buffer") shouldn't be removed.

wesm

It's good to make all this more precise + clean things up. I left some comments / questions

wesm · 2018-07-17T03:59:53Z

cpp/src/arrow/adapters/orc/adapter.cc

I was also confused to see a stack allocation here and above. Is it not possible to use source in AppendValues?

wesm · 2018-07-17T04:00:22Z

cpp/src/arrow/adapters/orc/adapter.cc

stack allocation here

wesm · 2018-07-17T04:02:11Z

cpp/src/arrow/buffer.h

I googled a bit. "fixed size" seems more common, and reads OK to me

wesm · 2018-07-17T04:03:00Z

cpp/src/arrow/builder.cc

I just left a review + comments there. I think we should try to merge this before merging #2243 so we can decide what to do there

wesm · 2018-07-17T04:04:14Z

cpp/src/arrow/builder.h

Are these lines equivalent (I thought so)?

Yes, I changed it back.

wesm · 2018-07-17T04:04:55Z

cpp/src/arrow/builder.h

wesm · 2018-07-17T04:05:01Z

cpp/src/arrow/builder.h

wesm · 2018-07-17T04:05:32Z

cpp/src/arrow/builder.h

If this is trivial, it can be omitted altogether?

wesm · 2018-07-17T04:06:37Z

cpp/src/arrow/python/python_to_arrow.cc

@pcmoritz or @robertnishihara take a look to make sure these changes look right.

Should this be Append(false)? I would expect nones_ to be an all non-null boolean array

Union array expects a null_count in addition to the null_bitmap. Since the boolean array doesn't track the number of true/false values, we add a null here. We could of course count falses manually after finishing.

That's the reason why I though the NullBuilder behaviour which I had previously made sense: you could add both null and non-null values and then extract the null bitmap and the null count.

wesm · 2018-07-17T04:08:02Z

python/pyarrow/tests/test_builder.py

Is this still needed?

alendit · 2018-07-18T10:18:03Z

So, what orc adapter did was to use std::copy to copy data from int64_t* input to a Int32 builder by specifying data() as a target. This works because std::copy takes iterators and they just need to conform to the interator interface. IThe builders, on the other hand, either accept a vector or a raw pointer, both have to have the exact expected type to work.

My solution is to overload the AppendValues method to accept iterators. Then we can just use std::copy internally. The number of existing overloads with all possible combination of pointer and vector arguments suggests that we might want to switch to iterator-accepting interfaces at some point in the future, since they are more general. In the ideal world we'd have ranges, but alas...

An additional problem was that orc adapter used data pointer to directly add a computed value (time in nanoseconds). What is easily done in python with generators, is not supported in any current C++ version directly (excluding non-standard extensions). I added the LazyRange, a thin wrapper around a callable (e.g. lambda) which returns objects conforming to the iterator interface. What looks like a complicated construct, can be optimized by the compiler very well (I added some benchmarks).

I really think we should remove/deprecate data() and null_bitmap() from the builders, but it's apparent, we'd need some interface restructuring. I think the changes would be beneficial in the long term, but I'm also not happy about the size of this diff. I would suggest that as long as the general direction is acceptable, we fix this PR's code and leave the more farreaching consideration to be fixed in further PRs.

alendit · 2018-07-18T13:53:23Z

Ok, so funny thing: MSVC14 (i.e. 2015) issues warning on narrowing conversion when using std::copy. The funny part is that none other compiler, ~~not even later MSVC versions~~, seem to be bothered by it.

Which raises the question, if the orc adapter in its current form has even been through CI, because [here|https://github.com/apache/arrow/blob/master/cpp/src/arrow/adapters/orc/adapter.cc#L535] it totaly copies int64_t to int32_t and smaller. Of course, it's not a problem, since we check the supplied type beforehand, but it shouldn't compile with MSVC 2015.

With the supplied changes it's actually pretty easy to fix the warning: just wrap the iterator with a lambda which does a static_cast to the right type. Should I do it? Should be 0 costs when compiled on Release.

An alternative would be to silence the warning, since it's apparently absolutely non-standard.

EDIT: Later MSVC do raise the same warning. gcc and clang aren't bothered, even on -Wall -Wextra -pedantic.

pitrou · 2018-07-18T16:11:45Z

@alendit can you confirm when the PR is ready for review again? Thanks for working on this!

wesm · 2018-07-18T20:41:03Z

I would suggest that as long as the general direction is acceptable, we fix this PR's code and leave the more farreaching consideration to be fixed in further PRs.

Sounds good to me

wesm · 2018-07-19T17:46:40Z

Reviewing this again now

wesm

+1, wow thanks @alendit for going to the effort of adding iterator versions for all the AppendValues functions, definitely going above and beyond. This will be really helpful going forward.

I took a pretty careful read through this and it looks good to me. @pitrou do you want to look any more or do you mind if I go ahead and merge? #2243 will need to be rebased after but it shouldn't be too bad

wesm · 2018-07-19T17:47:50Z

cpp/src/arrow/adapters/orc/adapter.cc

-    target_type* target = reinterpret_cast<target_type*>(builder->data()->mutable_data());
+    auto cast_iter = internal::MakeLazyRange(
+        [&source](int64_t index) { return static_cast<target_type>(source[index]); },
+        length);


wesm · 2018-07-19T17:53:14Z

cpp/src/arrow/builder.h

+    int64_t bit_offset = length_ % 8;
+    uint8_t bitset = null_bitmap_data_[byte_offset];
+
+    for (auto iter = begin; iter != end; ++iter) {


As an aside, would you expect the optimized code to be significantly different here vs. the original const uint8_t* valid_bytes version?

With STL iterators and raw pointers it should be exactly the same. With more complicated iterators, it's hard to tell, though the LazyRange ones give the same performance as loops, at least on GCC.

pitrou · 2018-07-18T12:43:28Z

cpp/src/arrow/builder.h

I don't think returning a const value makes sense? Either return a value or a const reference;

Since these are always C types I would think that just value_type is fine; I am not an expert on the fine details, though

Yupp, the first const should go.

pitrou · 2018-07-19T18:13:22Z

cpp/src/arrow/util/lazy-benchmark.cc

+
+BENCHMARK(BM_lazy_copy)->Repetitions(3)->Unit(benchmark::kMillisecond);
+
+// std::copy with a lazy iterator which does static cast


I am curious: what do you call a lazy iterator? ISTM iterators are always lazy.

I believe he's referring to the object returned by internal::MakeLazyRange

Yes, the LazyRange is lazy about calling the supplied generator function instead of generating a new container eagerly like std::generate, for example.

With these benchmarks I wanted to make sure, that the compilers can inline the lambdas properly so we don't sacrifice performance compared to a loop.

EDIT: reading this again, maybe it should be a lazy container to avoid confusion.

pitrou · 2018-07-19T18:23:03Z

By the way, these were just nits in case @alendit want to gold-plate it. I'm ok for this to go in :-)

wesm · 2018-07-19T18:37:51Z

👍 ok, I'm merging so we can get your other PR rebased. Thanks @alendit @pitrou for your careful attention on this issue

alendit · 2018-07-19T19:06:23Z

Nice, glad it went through. Thx both of you for the help and patience :)

Once further question: for really small changes, like removing that one const, do I open a JIRA issue, too? Or is there a simplified procedure, so to not to swamp the queue?

wesm · 2018-07-19T19:14:56Z

You can open a PR but don't bother creating a JIRA. I'll merge once the build runs

… comments Few changes we discussed in the PR #2239. Author: Dimitri Vorona <vorona@in.tum.de> Closes #2293 from alendit/zero-padding-addendum and squashes the following commits: b5f2a8a <Dimitri Vorona> Revert "Fix type cast" c0aed06 <Dimitri Vorona> Fix type cast cb1e8dd <Dimitri Vorona> Remove the unneeded const qualifier and clarify the comments

alendit force-pushed the zero-padding branch 4 times, most recently from 54282ec to 372c5e2 Compare July 11, 2018 17:36

pitrou requested changes Jul 12, 2018

View reviewed changes

pitrou changed the title ~~[C++] Zero padding bytes in PoolBuffer~~ ARROW-2822: [C++] Zero padding bytes in PoolBuffer Jul 12, 2018

wesm reviewed Jul 12, 2018

View reviewed changes

alendit force-pushed the zero-padding branch from 8e3f7b9 to 5e4d5e7 Compare July 16, 2018 07:34

alendit force-pushed the zero-padding branch from 04d0f92 to 5cc6df3 Compare July 16, 2018 10:58

pitrou requested changes Jul 16, 2018

View reviewed changes

wesm reviewed Jul 17, 2018

View reviewed changes

alendit force-pushed the zero-padding branch 3 times, most recently from 2811817 to 9ad72a4 Compare July 19, 2018 11:44

Dimitri Vorona added 7 commits July 19, 2018 15:46

Add buffer zeroing on allocation and fix copy

a2f5663

Prefer using AllocateBuffer, zero padding where it's impossible

bd1b3e4

Add warning on unfinished buffers and fix some problems

d2f0db7

Don't pass shrink_to_fit

4cea9fc

Move finished state unsetting to UnsafeAppendBitmap

e035716

More consistency with padding zeroing

e90248c

Consume overflow dicts status

d1f0ddb

Dimitri Vorona added 14 commits July 19, 2018 15:46

Don't include benchmark_main in the benchmark

2906df2

msvc complains about unchecked iterators

f616ac5

Whoops, old includes

33b98c8

Second try to make msvc happy about iterators

8b681b0

Suppress msvc compiler warning about narrowing conversion

92d2b14

Better disable warning

924467c

Maybe like this?

3d3d55e

Try function scope suppression

44f02aa

Handle custom template cases

0a2c6cb

Formatting

ee591f4

Convert to the correct type

bca761f

Fix lint warning about nullptr

4e829cb

More consisten handling of null values

e200072

Handle nullptr for iterators

3fd5cbb

alendit force-pushed the zero-padding branch from 9ad72a4 to 3c3aef7 Compare July 19, 2018 13:47

More precise nullptr use detection

27347bc

alendit force-pushed the zero-padding branch from 3c3aef7 to 27347bc Compare July 19, 2018 14:45

Cleanup

95bae66

wesm mentioned this pull request Jul 19, 2018

ARROW-2744: [C++] Avoid creating list arrays with a null values buffer #2243

Closed

wesm approved these changes Jul 19, 2018

View reviewed changes

pitrou reviewed Jul 19, 2018

View reviewed changes

pitrou approved these changes Jul 19, 2018

View reviewed changes

wesm closed this in aedba2c Jul 19, 2018

alendit mentioned this pull request Jul 19, 2018

ARROW-2822: [C++] Remove the unneeded const qualifier and clarify the comments #2293

Closed


		BENCHMARK(BM_lazy_copy)->Repetitions(3)->Unit(benchmark::kMillisecond);

		// std::copy with a lazy iterator which does static cast

ARROW-2822: [C++] Zero padding bytes in PoolBuffer #2239

ARROW-2822: [C++] Zero padding bytes in PoolBuffer #2239

Uh oh!

Conversation

alendit commented Jul 10, 2018

Uh oh!

codecov-io commented Jul 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pitrou 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!

alendit Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

alendit Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

alendit commented Jul 12, 2018

Uh oh!

alendit commented Jul 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jul 10, 2018 •

edited

Loading

alendit Jul 12, 2018 •

edited

Loading

alendit Jul 12, 2018 •

edited

Loading

alendit commented Jul 13, 2018 •

edited

Loading