[SPARK-7251] Perform sequential scan when iterating over BytesToBytesMap #6159

JoshRosen · 2015-05-14T22:04:33Z

This patch modifies BytesToBytesMap.iterator() to iterate through records in the order that they appear in the data pages rather than iterating through the hashtable pointer arrays. This results in fewer random memory accesses, significantly improving performance for scan-and-copy operations.

This is possible because our data pages are laid out as sequences of [keyLength][data][valueLength][data] entries. In order to mark the end of a partially-filled data page, we write -1 as a special end-of-page length (BytesToByesMap supports empty/zero-length keys and values, which is why we had to use a negative length).

This patch incorporates / closes #5836.

…ytesToBytesMap

AmplabJenkins · 2015-05-14T22:07:10Z

Merged build triggered.

AmplabJenkins · 2015-05-14T22:07:20Z

Merged build started.

SparkQA · 2015-05-14T22:08:01Z

Test build #32737 has started for PR 6159 at commit 273b842.

JoshRosen · 2015-05-14T22:09:35Z

Here's some benchmark code which builds a map with millions of records that have random 64-byte keys and values, then repeatedly iterates over the map and copies the keys and values into byte arrays: https://gist.github.com/JoshRosen/286b26494ab27e657051

For this benchmark, I saw a nearly 10x improvement as a result of this patch.

SparkQA · 2015-05-14T23:56:03Z

Test build #32737 has finished for PR 6159 at commit 273b842.

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

AmplabJenkins · 2015-05-14T23:56:07Z

Merged build finished. Test PASSed.

AmplabJenkins · 2015-05-14T23:56:08Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/32737/
Test PASSed.

JoshRosen · 2015-05-16T20:50:52Z

/cc @rxin for review.

rxin · 2015-05-17T01:47:57Z

@zsxwing can you take a look at this one as well?

zsxwing · 2015-05-18T06:15:31Z

@JoshRosen could you explain what will happen if oldCapacity == (1 << 30) in https://github.com/JoshRosen/spark/blob/SPARK-7251/unsafe/src/main/java/org/apache/spark/unsafe/map/BytesToBytesMap.java#L570 ? I think it will crash because of overflow. Right? It will pass ~~-1~~ -2147483648 to allocate.

zsxwing · 2015-05-18T06:25:14Z

unsafe/src/main/java/org/apache/spark/unsafe/map/BytesToBytesMap.java

The current implementation does not follow the javadoc of Iterator: @throws NoSuchElementException if the iteration has no more elements. However, it may be fine since it's an internal API.

If we decide to do this, there's other places where we should also perform the same change. I'd like to defer this decision for now and deal with it in a follow up patch that makes the change more broadly.

zsxwing · 2015-05-18T06:37:07Z

How about make BytesToBytesMap implement Iterable<BytesToBytesMap.Location> so that it can be used in for-each?

zsxwing · 2015-05-18T06:38:07Z

unsafe/src/test/java/org/apache/spark/unsafe/map/AbstractBytesToBytesMapSuite.java

Could you also add a test to iterate an empty BytesToBytesMap?

Good idea. I'll fold this into the emptyMap() test at the start of this file.

JoshRosen · 2015-05-18T21:49:08Z

@zsxwing, nice catch RE: the risk of overflow when growing the map. This is a really hard-to-hit corner-case, but it's still worth fixing. I'll address this shortly.

JoshRosen · 2015-05-18T22:34:04Z

I'm adding a test for the overflow and noticed something interesting: when we overflow to -2147483648 and pass that value to allocate, this case ends up being handled in Math.max in allocate(), so we end up allocating an array of size 64, which should end up causing problems in rehashing. It looks like the Math.max was a carryover from Reynold's original LongToLong map: https://github.com/rxin/jvm-unsafe-utils/blame/master/core/src/main/java/com/databricks/unsafe/util/LongToLongMap.java#L184. I'll strengthen the internal assertions to catch this.

JoshRosen · 2015-05-18T22:39:36Z

Ah, I see now that it has to be at least 64 so that the division when sizing the bitset is safe. I'll leave a comment explaining this.

JoshRosen · 2015-05-18T22:55:29Z

While fixing this, I found a units / sizing issue: our longArray can only contain Integer.MAX_VALUE elements and each map entry requires two entires in longArray, so our actual maximum capacity (# of supported map elements ) should be Integer.MAX_VALUE / 2. I'm fixing this and adding additional tests for the various boundary conditions near the maximum size.

AmplabJenkins · 2015-05-18T23:42:12Z

Merged build triggered.

AmplabJenkins · 2015-05-18T23:42:18Z

Merged build started.

SparkQA · 2015-05-18T23:44:20Z

Test build #33030 has started for PR 6159 at commit bc4854b.

zsxwing · 2015-05-18T23:59:50Z

unsafe/src/main/java/org/apache/spark/unsafe/map/BytesToBytesMap.java

MAX_CAPACITY should be (1 << 30) - 1.

The hashtable has to be power-of-2-sized. Our long array can have at most (1 << 30) elements, since that's the largest power-of-2 that's less than Integer.MAX_VALUE, but we need two long array entries per record, so that gives us a maximum capacity of (1 << 29).

SparkQA · 2015-05-19T01:27:55Z

Test build #33030 has finished for PR 6159 at commit bc4854b.

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

AmplabJenkins · 2015-05-19T01:28:00Z

Merged build finished. Test PASSed.

AmplabJenkins · 2015-05-19T01:28:01Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/33030/
Test PASSed.

AmplabJenkins · 2015-05-19T21:47:11Z

Merged build triggered.

AmplabJenkins · 2015-05-19T21:47:19Z

Merged build started.

SparkQA · 2015-05-19T21:49:31Z

Test build #33100 has started for PR 6159 at commit 05bd90a.

SparkQA · 2015-05-19T23:34:15Z

Test build #33100 has finished for PR 6159 at commit 05bd90a.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- public class TaskMemoryManager

AmplabJenkins · 2015-05-19T23:34:20Z

Merged build finished. Test PASSed.

AmplabJenkins · 2015-05-19T23:34:20Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/33100/
Test PASSed.

zsxwing · 2015-05-19T23:47:34Z

LGTM

JoshRosen · 2015-05-20T23:42:28Z

Thanks for the review. I'm going to merge this into master and branch-1.4 (1.4.0).

This patch modifies `BytesToBytesMap.iterator()` to iterate through records in the order that they appear in the data pages rather than iterating through the hashtable pointer arrays. This results in fewer random memory accesses, significantly improving performance for scan-and-copy operations. This is possible because our data pages are laid out as sequences of `[keyLength][data][valueLength][data]` entries. In order to mark the end of a partially-filled data page, we write `-1` as a special end-of-page length (BytesToByesMap supports empty/zero-length keys and values, which is why we had to use a negative length). This patch incorporates / closes #5836. Author: Josh Rosen <[email protected]> Closes #6159 from JoshRosen/SPARK-7251 and squashes the following commits: 05bd90a [Josh Rosen] Compare capacity, not size, to MAX_CAPACITY 2a20d71 [Josh Rosen] Fix maximum BytesToBytesMap capacity bc4854b [Josh Rosen] Guard against overflow when growing BytesToBytesMap f5feadf [Josh Rosen] Add test for iterating over an empty map 273b842 [Josh Rosen] [SPARK-7251] Perform sequential scan when iterating over entries in BytesToBytesMap (cherry picked from commit f2faa7a) Signed-off-by: Josh Rosen <[email protected]>

This patch modifies `BytesToBytesMap.iterator()` to iterate through records in the order that they appear in the data pages rather than iterating through the hashtable pointer arrays. This results in fewer random memory accesses, significantly improving performance for scan-and-copy operations. This is possible because our data pages are laid out as sequences of `[keyLength][data][valueLength][data]` entries. In order to mark the end of a partially-filled data page, we write `-1` as a special end-of-page length (BytesToByesMap supports empty/zero-length keys and values, which is why we had to use a negative length). This patch incorporates / closes apache#5836. Author: Josh Rosen <[email protected]> Closes apache#6159 from JoshRosen/SPARK-7251 and squashes the following commits: 05bd90a [Josh Rosen] Compare capacity, not size, to MAX_CAPACITY 2a20d71 [Josh Rosen] Fix maximum BytesToBytesMap capacity bc4854b [Josh Rosen] Guard against overflow when growing BytesToBytesMap f5feadf [Josh Rosen] Add test for iterating over an empty map 273b842 [Josh Rosen] [SPARK-7251] Perform sequential scan when iterating over entries in BytesToBytesMap

[SPARK-7251] Perform sequential scan when iterating over entries in B…

273b842

…ytesToBytesMap

JoshRosen mentioned this pull request May 14, 2015

[SPARK-7251][Spark Core] Perform sequential scan when iterating over entries in BytesToBytesMap #5836

Closed

zsxwing reviewed May 18, 2015
View reviewed changes

Add test for iterating over an empty map

f5feadf

Guard against overflow when growing BytesToBytesMap

bc4854b

zsxwing reviewed May 18, 2015
View reviewed changes

Fix maximum BytesToBytesMap capacity

2a20d71

Compare capacity, not size, to MAX_CAPACITY

05bd90a

asfgit closed this in f2faa7a May 20, 2015

JoshRosen deleted the SPARK-7251 branch May 26, 2015 03:40

[SPARK-7251] Perform sequential scan when iterating over BytesToBytesMap #6159

[SPARK-7251] Perform sequential scan when iterating over BytesToBytesMap #6159

Uh oh!

Conversation

JoshRosen commented May 14, 2015

Uh oh!

AmplabJenkins commented May 14, 2015

Uh oh!

AmplabJenkins commented May 14, 2015

Uh oh!

SparkQA commented May 14, 2015

Uh oh!

JoshRosen commented May 14, 2015

Uh oh!

SparkQA commented May 14, 2015

Uh oh!

AmplabJenkins commented May 14, 2015

Uh oh!

AmplabJenkins commented May 14, 2015

Uh oh!

JoshRosen commented May 16, 2015

Uh oh!

rxin commented May 17, 2015

Uh oh!

zsxwing commented May 18, 2015

Uh oh!

zsxwing May 18, 2015

Choose a reason for hiding this comment

Uh oh!

JoshRosen May 18, 2015

Choose a reason for hiding this comment

Uh oh!

zsxwing commented May 18, 2015

Uh oh!

zsxwing May 18, 2015

Choose a reason for hiding this comment

Uh oh!

JoshRosen May 18, 2015

Choose a reason for hiding this comment

Uh oh!

JoshRosen commented May 18, 2015

Uh oh!

JoshRosen commented May 18, 2015

Uh oh!

JoshRosen commented May 18, 2015

Uh oh!

JoshRosen commented May 18, 2015

Uh oh!

AmplabJenkins commented May 18, 2015

Uh oh!

AmplabJenkins commented May 18, 2015

Uh oh!

SparkQA commented May 18, 2015

Uh oh!

zsxwing May 18, 2015

Choose a reason for hiding this comment

Uh oh!

JoshRosen May 19, 2015

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 19, 2015

Uh oh!

AmplabJenkins commented May 19, 2015

Uh oh!

AmplabJenkins commented May 19, 2015

Uh oh!

AmplabJenkins commented May 19, 2015

Uh oh!

AmplabJenkins commented May 19, 2015

Uh oh!

SparkQA commented May 19, 2015

Uh oh!

SparkQA commented May 19, 2015

Uh oh!

AmplabJenkins commented May 19, 2015

Uh oh!

AmplabJenkins commented May 19, 2015

Uh oh!

zsxwing commented May 19, 2015

Uh oh!