[SPARK-15962][SQL] Introduce implementation with a dense format for UnsafeArrayData

[kiszk]

What changes were proposed in this pull request?

This PR introduces more compact representation for UnsafeArrayData.

UnsafeArrayData needs to accept null value in each entry of an array. In the current version, it has three parts

[numElements] [offsets] [values]

Offsets has the number of numElements, and represents null if its value is negative. It may increase memory footprint, and introduces an indirection for accessing each of values.

This PR uses bitvectors to represent nullability for each element like UnsafeRow, and eliminates an indirection for accessing each element. The new UnsafeArrayData has four parts.

[numElements][null bits][values or offset&length][variable length portion]

In the null bits region, we store 1 bit per element, represents whether an element is null. Its total size is ceil(numElements / 8) bytes, and it is aligned to 8-byte boundaries.
In the values or offset&length region, we store the content of elements. For fields that hold fixed-length primitive types, such as long, double, or int, we store the value directly in the field. For fields with non-primitive or variable-length values, we store a relative offset (w.r.t. the base address of the array) that points to the beginning of the variable-length field and length (they are combined into a long). Each is word-aligned. For variable length portion, each is aligned to 8-byte boundaries.

The new format can reduce memory footprint and improve performance of accessing each element. An example of memory foot comparison:
1024x1024 elements integer array
Size of baseObject for UnsafeArrayData: 8 + 1024x1024 + 1024x1024 = 2M bytes
Size of baseObject for UnsafeArrayData: 8 + 1024x1024/8 + 1024x1024 = 1.25M bytes

In summary, we got 1.0-2.6x performance improvements over the code before applying this PR.
Here are performance results of benchmark programs:

Read UnsafeArrayData: 1.7x and 1.6x performance improvements over the code before applying this PR

OpenJDK 64-Bit Server VM 1.8.0_91-b14 on Linux 4.4.11-200.fc22.x86_64
Intel Xeon E3-12xx v2 (Ivy Bridge)
Without SPARK-15962
Read UnsafeArrayData: Best/Avg Time(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------
Int 430 / 436 390.0 2.6 1.0X
Double 456 / 485 367.8 2.7 0.9X
With SPARK-15962
Read UnsafeArrayData: Best/Avg Time(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------
Int 252 / 260 666.1 1.5 1.0X
Double 281 / 292 597.7 1.7 0.9X

Write UnsafeArrayData: 1.0x and 1.1x performance improvements over the code before applying this PR

OpenJDK 64-Bit Server VM 1.8.0_91-b14 on Linux 4.0.4-301.fc22.x86_64
Intel Xeon E3-12xx v2 (Ivy Bridge)
Without SPARK-15962
Write UnsafeArrayData: Best/Avg Time(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------
Int 203 / 273 103.4 9.7 1.0X
Double 239 / 356 87.9 11.4 0.8X
With SPARK-15962
Write UnsafeArrayData: Best/Avg Time(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------
Int 196 / 249 107.0 9.3 1.0X
Double 227 / 367 92.3 10.8 0.9X

Get primitive array from UnsafeArrayData: 2.6x and 1.6x performance improvements over the code before applying this PR

OpenJDK 64-Bit Server VM 1.8.0_91-b14 on Linux 4.0.4-301.fc22.x86_64
Intel Xeon E3-12xx v2 (Ivy Bridge)
Without SPARK-15962
Get primitive array from UnsafeArrayData: Best/Avg Time(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------
Int 207 / 217 304.2 3.3 1.0X
Double 257 / 363 245.2 4.1 0.8X
With SPARK-15962
Get primitive array from UnsafeArrayData: Best/Avg Time(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------
Int 151 / 198 415.8 2.4 1.0X
Double 214 / 394 293.6 3.4 0.7X

Create UnsafeArrayData from primitive array: 1.7x and 2.1x performance improvements over the code before applying this PR

OpenJDK 64-Bit Server VM 1.8.0_91-b14 on Linux 4.0.4-301.fc22.x86_64
Intel Xeon E3-12xx v2 (Ivy Bridge)
Without SPARK-15962
Create UnsafeArrayData from primitive array: Best/Avg Time(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------
Int 340 / 385 185.1 5.4 1.0X
Double 479 / 705 131.3 7.6 0.7X
With SPARK-15962
Create UnsafeArrayData from primitive array: Best/Avg Time(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------
Int 206 / 211 306.0 3.3 1.0X
Double 232 / 406 271.6 3.7 0.9X

1.7x and 1.4x performance improvements in UDTSerializationBenchmark over the code before applying this PR

OpenJDK 64-Bit Server VM 1.8.0_91-b14 on Linux 4.4.11-200.fc22.x86_64
Intel Xeon E3-12xx v2 (Ivy Bridge)
Without SPARK-15962
VectorUDT de/serialization: Best/Avg Time(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------
serialize 442 / 533 0.0 441927.1 1.0X
deserialize 217 / 274 0.0 217087.6 2.0X
With SPARK-15962
VectorUDT de/serialization: Best/Avg Time(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------
serialize 265 / 318 0.0 265138.5 1.0X
deserialize 155 / 197 0.0 154611.4 1.7X

How was this patch tested?

Added unit tests into UnsafeArraySuite

[SparkQA]

Test build #60556 has finished for PR 13680 at commit 639b32d.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #60566 has finished for PR 13680 at commit d06d200.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #60568 has finished for PR 13680 at commit 6c09d72.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[cloud-fan]

Thanks for working on it! One of my concern is: do we really need 2 unsafe array implementations? For the UnsafeArrayDataDense, can we follow unsafe row and introduce a null-bits to make it support null value? Then we can have a unified format: [numElements] [null bits] [offset or primitive values] [values]

[kiszk]

@cloud-fan thank you for your good comment. I also read previous proposal.
I love to have only single format (or implementation). Since I thought that there are some reasons to keep the old format, I introduced a new dense format.

IMHO, a new unified format should have three properties.

1. Remove indirect offset (for performance and footprint)
2. Have capability of presence of nullbit (for generality)
3. Quickly get information on existence of null value in an array (for performance, in particular, primitive array)

Based on them, how about this single format?

[numElements] [all zero in null bits?] [null bits] [values] [variable length portion]

If we want to reduce memory footprint in the case of primitive array, we can drop [null bits] part if [all zero in null bits?] has a special value.

[cloud-fan]

null bits won't take a lot of memory(1 bit per element), and having the all zero in null bits? flag will slow down elements retrievement: we need an extra if branch to check this flag, and make the format more complex. I'd like to always have the null bits region, cc @davies

[kiszk]

It is OK to always keep [null bits]

One question: Is this format to keep fixed space for [values]? I mean if [null bit] is true, the corresponding element in [value] occupy space. It allow us to always access all of the elements by 4 + (null bits size) + (element size) * offset.

If an answer of the above question is yes, I agree with no [all zero in null bits?]. Then, I will revise this PR.

cc @hvanhovell

[SparkQA]

Test build #61095 has finished for PR 13680 at commit 85f862c.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[cloud-fan]

@kiszk yea, even the null bits is true, the element still take space at [offset or fixed-length values] region.

[kiszk]

Good to hear. I will make an implementation for single format. If I would meet some issues, I will raise them here.

[cloud-fan]

Thanks! Feel free to ask any questions!

[kiszk]

@cloud-fan , I have one question about null field. Should we put zero into the corresponding field to position where setNullAt() is called as UnsafeRowdoes.

If we avoid to put zero, this avoidance affects two properties.

1. row equality
2. undetermined value may be included in the array returned by UnsafeArrayData.to<PrimitiveType>Array()

In my current implementation, a width of each element depends on element type (4: Int, 8: Double, etc). Thus, it is hard to do the same approach as it did. Since UnsafeRow always use 8 bytes per field. Since we want to make data conversion fast between primitive array and unsafe array, we have to keep the type-based element-width (e.g. 4: Int, 8: Double, etc).

What do you think? Should we have to keep the above two property by clearing a field?

[kiszk]

One potential performance issue is that we have to always clear all of null bits at UnsafeArrayWriter.initialize(). IIUC, this is because holder.buffer is reused for each row. If one row has more than one variable length arrays, holder.cursor may have different offset for the same array among different rows. Thus, we have to clear all of null bits.
This overhead would be larger in machine learning use cases that use large arrays.

If we have a flag to show whether there is an area of [null bits] or we have two implementations with [null bits] and without [null bits], we may alleviate performance overhead.

What do you think?
cc: @hvanhovell , @mengxr

[cloud-fan]

@kiszk we should definitely put zero into the corresponding field when set null. It will be a little harder than UnsafeRow, as we need setNullBoolean, setNullInt, etc. but it's still doable.

about clear out all null bits, yea, it's a big overhead for array with small element like boolean array, but I'm not sure this worth 2 different implementations, cc @rxin

[kiszk]

@cloud-fan , for the first issue, we are on the same page. Your proposal is what I am thinking about as possible solutions. I will do that.

For the second issue, it seems to be design choice between

1. introduce one conditional branch in isNullAt() in one implementation
2. have two implementations without conditional branch at isNullAt()

[cloud-fan]

having 2 implementations is also kind of a branch: the virtual function call need to be dispatched between these 2 implementations, while the only one implementation can be marked as final and doesn't have this overhead.

[kiszk]

I see. I assumed that virtual call will be devirtualized by declaring final method and by optimistically propagating type information in the JIT compiler. Would it be better to add a flag like containNull ?

[cloud-fan]

I don't have a strong preference here, each choice has its advantage and weakness:

1. alway have the null bits region: faster element access(read the null bits and then read the element), slower writing(always need to clear out the null bit region for primitive array)
2. only have null bits region if containsNull flag is true: slower element access(read flag then read null bits then read the element), faster writing(check the containsNull flag and don't need to clear out null bits if flag is false)

cc @davies

[hvanhovell]

I am kind in favor of the single implementation for a couple of reasons:

• Declaring methods final is not a magic bullet. If you are invokingisNull or get* on the common ancestor it is still a virtual call. If you are only using two classes the JVM is clever enough to add a branch in the code and call one of the two methods depending of the type (it is probably also clever enough to inline the methods). We could get around this if we can use the concrete type during code generation (which should be doable).
• Nulling out the null region is IMO not really a problem. JVMs nowadays are very good at optimizing array fill like operations. See this post for some more information: http://psy-lob-saw.blogspot.com/2015/04/on-arraysfill-intrinsics-superword-and.html
• On our side code generation should be clever enough not to do null checks for non-nullable fields (is it?). So we are not incurring useless branch here.

It would help to have a number of performance benchmarks.

[rxin]

We should do this more holistically, i.e. thinking about what we want to do with primitive arrays for machine learning and how to handle everything end to end. Let's not rush an implementation change just for the low level data structure.

[SparkQA]

Test build #61218 has finished for PR 13680 at commit 517aa72.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #61222 has finished for PR 13680 at commit bf12e72.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #61233 has finished for PR 13680 at commit 682c397.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #61235 has finished for PR 13680 at commit 687be4d.

• This patch fails Scala style tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #61236 has finished for PR 13680 at commit 138810b.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #61239 has finished for PR 13680 at commit 500e978.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[kiszk]

@cloud-fan and @hvanhovell thank you for your comments. Based on your comments, I implemented UnsafeArrayData by using one implementation with explicit clearing null bits by Arrays.fill().
I would appreciate it if you review this PR.

[kiszk]

Jenkins, retest this please

[SparkQA]

Test build #65920 has finished for PR 13680 at commit 2ef6e3b.

• This patch fails from timeout after a configured wait of 250m.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #65930 has finished for PR 13680 at commit 2ef6e3b.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[kiszk]

@cloud-fan could you please review this again?
I addressed your comments. After rebasing, the performance issue has been solved.

[cloud-fan]

thanks for your great work! merging to master!