Skip to content

[SPARK-32872][CORE] Prevent BytesToBytesMap at MAX_CAPACITY from exceeding growth threshold #29744

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ankurdave wants to merge 6 commits into from
Closed

[SPARK-32872][CORE] Prevent BytesToBytesMap at MAX_CAPACITY from exceeding growth threshold #29744

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c3afb09
[SPARK-32872][CORE] Prevent BytesToBytesMap from exceeding growth thr…
ankurdave Sep 13, 2020
de0f9fe
Reword comment based on review feedback
ankurdave Sep 13, 2020
7d76463
Better comment wording
ankurdave Sep 13, 2020
ec51e65
Adopt comment wording suggested by @viirya
ankurdave Sep 13, 2020
f610fbe
Address review comment
ankurdave Sep 14, 2020
680c4cf
Line length
ankurdave Sep 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions core/src/main/java/org/apache/spark/unsafe/map/BytesToBytesMap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -816,6 +816,10 @@ public boolean append(Object kbase, long koff, int klen, Object vbase, long voff
} catch (SparkOutOfMemoryError oom) {
canGrowArray = false;
}
} else if (numKeys >= growthThreshold && longArray.size() / 2 >= MAX_CAPACITY) {
Comment thread
ankurdave marked this conversation as resolved.
Outdated
// The map cannot grow because doing so would cause it to exceed MAX_CAPACITY. Instead, we
// prevent the map from accepting any more new elements.
canGrowArray = false;

@viirya viirya Sep 13, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we won't exceed MAX_CAPACITY at all. We have some restrictions to prevent it.

The root cause, I think, is once the map reaches MAX_CAPACITY, we won't grow the map even the numKeys is more than growthThreshold. So we could add elements to the map to more than load factor (0.5) of it. The map cannot be used for sorting later in UnsafeKVExternalSorter. Although UnsafeKVExternalSorter will allocate new array in the case, it could hit OOM and fails the query.

This change favors reusing the array for sorting, but it prevents the map accepting new elements more early, half of the array is not used after the map capacity reaches MAX_CAPACITY. But by doing this, we will do spilling early too.

When I was fixing the customer application before, we don't see the OOM even the payload reached MAX_CAPACITY. Is it possibly due to the memory setting on your payload/cluster?

@ankurdave ankurdave Sep 13, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Once we reach MAX_CAPACITY, we have a choice: we can either (a) exceed the load factor, or (b) we can spill.

The code currently does (a). If the number of groups ultimately turns out to be less than MAX_CAPACITY, then this may avoid spilling. (Performance will degrade as the load factor approaches 1.0, but it will still be better than spilling.) However, if the number of groups is larger than MAX_CAPACITY, or if we need more memory to store the records, then we will be unable to spill and the query will fail.

It seems to me that it would be better to prevent queries from failing, as (b) does. This does mean we would spill earlier for a narrow range of queries.

@ankurdave ankurdave Sep 13, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the memory setting: I agree that whether or not an OOM will occur is dependent on the memory setting, but I would argue that (1) the user should not be required to tune memory settings to avoid failing queries, and (2) many reasonable settings will cause an OOM.

The problem arises when there are between 2^28+1 and 2^29 keys. If we need to spill, UnsafeKVExternalSorter will attempt to allocate between 8 and 16 GiB. Therefore the user will either need to increase the memory per task by approximately this amount so that this allocation will succeed, or decrease the memory per task sufficiently that the map will never reach MAX_CAPACITY in the first place. This seems to create quite a wide range of problematic settings.

}
}
return true;
Expand Down