Update spatial index memory while building R-Tree #13251

viczhang861 · 2019-08-19T05:43:19Z

Update memory usage during construction of rtree

Before this change: large spatial query fail with internal communication error
After this change : same query failed with message "Query exceeded distributed user memory limit of 5TB"

== RELEASE NOTES ==

General Changes
-----------------
* Improve memory tracking for geospatial query indexing and prevent worker node down due to out of memory

jagill · 2019-08-19T14:40:45Z

presto-main/src/main/java/com/facebook/presto/operator/PagesSpatialIndexSupplier.java

+            // update memory size when 1000 more leaf nodes are inserted
+            if (position % 1000 == 999) {
+                localUserMemoryContext.setBytes(estimatedLeafNodesSize);
+            }
        }

        rtree.build();


What about the trailing geometries after the last localUserMemoryContext, and the cost of the built rtree? For the JTS rtree this is quite significant.

The overhead of non-leaf node is about 10% of leaf nodes. After SRTree is built, a final size check is already done on all nodes at all levels, which includes trailing geometries.

mbasmanova

@viczhang861 Vic, thanks for making this change. Updating memory usage while building the R-Tree will help avoid JVM OOMs and allow for proper memory cap enforcement. I'm curious how this is done for regular hash table. CC: @nezihyigitbasi

The commit title Monitor spatial index memory usage is misleading because this change doesn't do anything about the monitoring. I enabled the monitoring logic by starting to send timely updates to memory usage. I'd change the commit message to:

Update memory usage while building R-Tree

presto-main/src/main/java/com/facebook/presto/operator/PagesSpatialIndexSupplier.java

mbasmanova · 2019-08-19T17:54:18Z

presto-main/src/main/java/com/facebook/presto/operator/PagesSpatialIndexSupplier.java

+            estimatedLeafNodesSize = estimatedLeafNodesSize + ENVELOPE_INSTANCE_SIZE + geometryWithPosition.getEstimatedMemorySizeInBytes();
+
+            // update memory size when 1000 more leaf nodes are inserted
+            if (position % 1000 == 999) {


I think it would be better to update the memory every so many bytes (or megabytes) rather than every so many objects.

How about 100 MB? for a cluster with 600 nodes, this means > 60GB total user memory change per update.

mbasmanova · 2019-08-19T17:59:13Z

@viczhang861 Vic, is there a way to test this change in a unit test? CC: @nezihyigitbasi

aweisberg

I don't think this will end up doing what we want in terms of aborting if there isn't enough memory due to how setBytes works.

This is how I had to fix it https://github.com/prestodb/presto/compare/master...aweisberg:fix_spatial_index_accounting?expand=1

@arhimondr Can probably confirm, but basically you have to yield to the driver loop with the memory accounted for as each page is processed in order to get the query to halt if it uses too much memory.

viczhang861 · 2019-08-19T21:29:59Z

@aweisberg This diff was tested in a real cluster, I can share you query ids offline if you are interested

aweisberg · 2019-08-19T23:59:00Z

I'm also working on the memory accounting for the hash table associated with PagesIndex. It's not as bad because it tends to be smaller, but it's also built in one giant go with no memory accounting.

mbasmanova · 2019-08-20T04:47:02Z

you have to yield to the driver loop with the memory accounted for as each page is processed in order to get the query to halt if it uses too much memory.

@nezihyigitbasi Nezih, could you clarify how memory accounting/limit enforcement works?

aweisberg · 2019-08-20T15:01:45Z

@viczhang861 Can you try 20190806_050942_00000_yrda6 and see how it does?

viczhang861 · 2019-08-20T15:14:37Z

you have to yield to the driver loop with the memory accounted for as each page is processed in order to get the query to halt if it uses too much memory.

I don't understand what this means, but let me explain why this PR is working.

SpatialIndexBuilderOperator updates memory when each page is added (query will be killed if memory budget exceeds at this step) or when R-Tree build is done (query could also be killed at this step). However, memory is not updated when R-Tree is built. Memory will increase significantly when data is deserialized and geometry objects are created by JTS library.

We don't need to worry about setBytes, which is already doing the right thing, otherwise, query will not be killed in above mentioned steps. For this reason, unit test on behaviors of calling setBytes is not the scope of this PR.

viczhang861 · 2019-08-20T15:49:10Z

@aweisberg This is exactly the query I tested. Query id after fixing: 20190819_051830_00005_hpdfs

aweisberg · 2019-08-20T17:14:52Z

I don't follow why this works. Doesn't mean that it doesn't though.

That query builds a 180+ gigabyte tree on one node that causes an OOM. Calling setBytes won't fix that because it should still go ahead and build the entire tree and OOM the worker. Even if the coordinator detects the query exceeding the limits and returns an error to the client saying it killed the query (is killing asynchronous?) we still want to avoid OOMing the worker.

The coordinator can't kill the query as it is building the tree. The operator is in a loop that won't be interrupted as far as I can tell.

I checked the logs and couldn't really tell if the workers had OOMed. There are eventually a spate of worker restarts but exit code is 0 and there is no message. This is not typical of what I have seen for OOMs, but maybe this cluster was configured differently?

If it does work, great! I just want to make sure we aren't missing something because it's an unexpected result to me.

viczhang861 · 2019-08-20T17:27:26Z

@aweisberg I monitored heap memory usage on worker nodes before and after fix, the worker nodes didn't crash after fix. I assume that is the result of killing the query due to "Exceeded global user memory limit of ***"

viczhang861 · 2019-08-20T17:35:53Z

@aweisberg Another good signal is that worker didn't restart after the fix.

aweisberg · 2019-08-20T17:45:39Z

presto-main/src/main/java/com/facebook/presto/operator/PagesSpatialIndexSupplier.java

+            GeometryWithPosition geometryWithPosition = new GeometryWithPosition(ogcGeometry, partition, position);
+            rtree.insert(getJtsEnvelope(ogcGeometry, radius), geometryWithPosition);
+
+            estimatedMemorySizeInBytes += ENVELOPE_INSTANCE_SIZE + geometryWithPosition.getEstimatedMemorySizeInBytes();


There is an array of references collecting the geometries inside the rtree. It's not 100% accurate, but can you add size of an object reference each time as well?

Could you be more specific about which array ? I only include leaf nodes into consideration, the total size will be off by 11%.

Is there a reason not to be as precise as we can?

The rtree before you invoke build is wrapping what you provide in an ItemBoundable and stashing it in a big array which it then discards when the rtree is built.

This is a slightly more accurate accounting https://github.com/aweisberg/presto/blob/fix_spatial_index_accounting/presto-main/src/main/java/com/facebook/presto/operator/PagesSpatialIndexBuilder.java#L143

11% more nodes are generated when insertion is done. I will add ItemBoundable. What is the 8-bytes array? reference to new element in ItemBoundable array?

What file/line in the rtree library generates more nodes on insertion? When I looked at the rtree code it just wraps it and stores it in an array.

Do you mean it uses more after it builds?

https://github.com/locationtech/jts/blob/master/modules/core/src/main/java/org/locationtech/jts/index/strtree/AbstractSTRtree.java#L243
https://github.com/locationtech/jts/blob/master/modules/core/src/main/java/org/locationtech/jts/index/strtree/AbstractSTRtree.java#L109

Do you mean it uses more after it builds?

Yes, when insertion is done, parent nodes will be created after final build

createHigherLevels https://github.com/locationtech/jts/blob/master/modules/core/src/main/java/org/locationtech/jts/index/strtree/AbstractSTRtree.java#L107

aweisberg · 2019-08-20T17:46:52Z

presto-main/src/main/java/com/facebook/presto/operator/PagesSpatialIndexSupplier.java

+
+            // update memory size when estimated change is more than 100 MB
+            if (estimatedMemorySizeInBytes - lastUpdatedMemorySizeInBytes >= 100_000_000) {
+                localUserMemoryContext.setBytes(estimatedMemorySizeInBytes);


This is dropping the memory used by the PagesIndex itself?

Thanks, I included PagesIndex in

aweisberg

LGTM

mbasmanova

@viczhang861 Looks good overall % some comments.

presto-main/src/main/java/com/facebook/presto/operator/PagesSpatialIndexSupplier.java

mbasmanova · 2019-08-21T16:53:26Z

presto-main/src/main/java/com/facebook/presto/operator/PagesSpatialIndexSupplier.java

+            GeometryWithPosition geometryWithPosition = new GeometryWithPosition(ogcGeometry, partition, position);
+            rtree.insert(getJtsEnvelope(ogcGeometry, radius), geometryWithPosition);
+
+            // 8-bytes for reference to the new element in itemBoundables array of STRtree


This is an implementation detail of the STRtree and therefore shouldn't be used here. How about just tracking size of the envelope and geometry?

private static final int MEMORY_USAGE_UPDATE_INCREMENT_BYTES = 100 * 1024 * 1024; // 100 MB addedSizeInBytes += ENVELOPE_INSTANCE_SIZE + geometryWithPosition.getEstimatedMemorySizeInBytes(); if (addedSizeInBytes >= MEMORY_USAGE_UPDATE_INCREMENT_BYTES) { localUserMemoryContext.setBytes(recordedSizeInBytes + addedSizeInBytes); recordedSizeInBytes += addedSizeInBytes; addedSizeInBytes = 0; }

presto-main/src/main/java/com/facebook/presto/operator/PagesSpatialIndexSupplier.java

viczhang861 · 2019-08-22T15:39:57Z

@mbasmanova Thanks for review, all comments addressed and rebased to be compatible with FlatBush Rtree

mbasmanova

@viczhang861 Looks good to me.

arhimondr · 2019-08-23T14:48:17Z

Agree with @aweisberg , an Operator should return control to the engine between memory usage updates to ensure the memory manager works. No idea how it works.

@nezihyigitbasi , you have the most expertise in the memory management. Your input here will be greatly appreciated.

facebook-github-bot added the CLA Signed label Aug 19, 2019

viczhang861 force-pushed the spatial-query-memory branch from b1c2f0e to 718e21f Compare August 19, 2019 07:34

viczhang861 requested review from mbasmanova, mayankgarg1990, tastynoodle and aweisberg August 19, 2019 14:27

jagill reviewed Aug 19, 2019

View reviewed changes

mbasmanova requested changes Aug 19, 2019

View reviewed changes

viczhang861 force-pushed the spatial-query-memory branch from 718e21f to 0ed7ee1 Compare August 19, 2019 18:58

viczhang861 changed the title ~~Monitor spatial index memory~~ Update spatial index memory while building R-Tree Aug 19, 2019

viczhang861 requested a review from mbasmanova August 19, 2019 20:11

aweisberg reviewed Aug 19, 2019

View reviewed changes

viczhang861 assigned mbasmanova Aug 20, 2019

aweisberg suggested changes Aug 20, 2019

View reviewed changes

viczhang861 force-pushed the spatial-query-memory branch from 0ed7ee1 to 687d496 Compare August 20, 2019 18:50

viczhang861 requested a review from aweisberg August 20, 2019 20:03

aweisberg approved these changes Aug 20, 2019

View reviewed changes

mbasmanova requested changes Aug 21, 2019

View reviewed changes

viczhang861 force-pushed the spatial-query-memory branch from 687d496 to e107042 Compare August 22, 2019 14:48

Update memory usage while building R-Tree

f4367da

viczhang861 force-pushed the spatial-query-memory branch from e107042 to f4367da Compare August 22, 2019 15:36

viczhang861 requested a review from mbasmanova August 22, 2019 15:40

mbasmanova approved these changes Aug 22, 2019

View reviewed changes

mbasmanova merged commit 8492bfa into prestodb:master Aug 22, 2019

viczhang861 deleted the spatial-query-memory branch August 29, 2019 22:22

neeradsomanchi mentioned this pull request Sep 9, 2019

Release notes for 0.226 #13340

Closed

JunhyungSong mentioned this pull request Nov 16, 2021

Improve performance/memory overhead for Spatial Join trinodb/trino#9965

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update spatial index memory while building R-Tree #13251

Update spatial index memory while building R-Tree #13251

viczhang861 commented Aug 19, 2019 •

edited

Loading

jagill Aug 19, 2019

viczhang861 Aug 19, 2019

mbasmanova left a comment

mbasmanova Aug 19, 2019

viczhang861 Aug 19, 2019

mbasmanova commented Aug 19, 2019

aweisberg left a comment

viczhang861 commented Aug 19, 2019

aweisberg commented Aug 19, 2019

mbasmanova commented Aug 20, 2019

aweisberg commented Aug 20, 2019 •

edited

Loading

viczhang861 commented Aug 20, 2019

viczhang861 commented Aug 20, 2019

aweisberg commented Aug 20, 2019

viczhang861 commented Aug 20, 2019 •

edited

Loading

viczhang861 commented Aug 20, 2019

aweisberg Aug 20, 2019

viczhang861 Aug 20, 2019

aweisberg Aug 20, 2019 •

edited

Loading

viczhang861 Aug 20, 2019 •

edited

Loading

aweisberg Aug 20, 2019

aweisberg Aug 20, 2019

viczhang861 Aug 20, 2019 •

edited

Loading

viczhang861 Aug 20, 2019

aweisberg Aug 20, 2019

viczhang861 Aug 20, 2019

aweisberg left a comment

mbasmanova left a comment

mbasmanova Aug 21, 2019

viczhang861 commented Aug 22, 2019

mbasmanova left a comment

arhimondr commented Aug 23, 2019

Update spatial index memory while building R-Tree #13251

Update spatial index memory while building R-Tree #13251

Conversation

viczhang861 commented Aug 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbasmanova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbasmanova commented Aug 19, 2019

aweisberg left a comment

Choose a reason for hiding this comment

viczhang861 commented Aug 19, 2019

aweisberg commented Aug 19, 2019

mbasmanova commented Aug 20, 2019

aweisberg commented Aug 20, 2019 • edited Loading

viczhang861 commented Aug 20, 2019

viczhang861 commented Aug 20, 2019

aweisberg commented Aug 20, 2019

viczhang861 commented Aug 20, 2019 • edited Loading

viczhang861 commented Aug 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aweisberg Aug 20, 2019 • edited Loading

Choose a reason for hiding this comment

viczhang861 Aug 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viczhang861 Aug 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aweisberg left a comment

Choose a reason for hiding this comment

mbasmanova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viczhang861 commented Aug 22, 2019

mbasmanova left a comment

Choose a reason for hiding this comment

arhimondr commented Aug 23, 2019

viczhang861 commented Aug 19, 2019 •

edited

Loading

aweisberg commented Aug 20, 2019 •

edited

Loading

viczhang861 commented Aug 20, 2019 •

edited

Loading

aweisberg Aug 20, 2019 •

edited

Loading

viczhang861 Aug 20, 2019 •

edited

Loading

viczhang861 Aug 20, 2019 •

edited

Loading