Fix deadlock and simplify proxy tracking #712

madsbk · 2021-08-26T11:06:15Z

This PR introduce a ProxyManager that replaces the current implementation of proxy tracking:

class ProxyManager:
    """
    This class together with Proxies, ProxiesOnHost, and ProxiesOnDevice
    implements the tracking of all known proxies and their total host/device
    memory usage. It turns out having to re-calculate memory usage continuously
    is too expensive.

    The idea is to have the ProxifyHostFile or the proxies themself update
    their location (device or host). The manager then tallies the total memory usage.

    Notice, the manager only keeps weak references to the proxies.
    """

Additionally, this PR fixes a rare deadlock by having all proxies and the ProxyManager use the same lock. Finally, this PR will make it much easier to implement spilling to disk: #708.

Notice, from the user's perspective, this PR shouldn't change anything.

codecov-commenter · 2021-08-27T12:14:38Z

Codecov Report

Merging #712 (4225364) into branch-21.10 (8e6ab70) will decrease coverage by 1.38%.
The diff coverage is 87.30%.

@@               Coverage Diff                @@
##           branch-21.10     #712      +/-   ##
================================================
- Coverage         87.63%   86.25%   -1.39%     
================================================
  Files                15       15              
  Lines              1658     1717      +59     
================================================
+ Hits               1453     1481      +28     
- Misses              205      236      +31

Impacted Files	Coverage Δ
dask_cuda/cuda_worker.py	`77.64% <ø> (ø)`
dask_cuda/get_device_memory_objects.py	`70.00% <0.00%> (+1.94%)`	⬆️
dask_cuda/local_cuda_cluster.py	`77.88% <50.00%> (ø)`
dask_cuda/utils.py	`81.10% <55.26%> (-6.18%)`	⬇️
dask_cuda/proxify_host_file.py	`92.96% <91.50%> (-6.44%)`	⬇️
dask_cuda/proxy_object.py	`89.51% <98.11%> (-0.13%)`	⬇️
dask_cuda/device_host_file.py	`70.00% <100.00%> (-0.17%)`	⬇️
dask_cuda/explicit_comms/dataframe/shuffle.py	`98.03% <100.00%> (ø)`
dask_cuda/initialize.py	`88.88% <100.00%> (ø)`
dask_cuda/is_device_object.py	`62.96% <100.00%> (ø)`
... and 4 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 a077463...4225364. Read the comment docs.

In order to address ambiguity when serializing using multiple serializers, we disallow proxies of collections such as list or tuples. Instead, users should wrap each collection item in a proxy.

pentschev

Overall looks good, great work @madsbk . I have only a few minor comments/suggestions.

dask_cuda/proxify_device_objects.py

dask_cuda/proxify_host_file.py

pentschev · 2021-09-07T13:24:36Z

dask_cuda/proxify_host_file.py

+            for dev_buf, proxies in self.get_dev_buffer_to_proxies().items():
+                last_access = max(p._obj_pxy.get("last_access", 0) for p in proxies)
+                size = sizeof(dev_buf)
+                dev_buf_access.append((last_access, size, proxies))
+                total_dev_mem_usage += size


I don't know how expensive this is, but would it make sense to update this information when there's a change instead (i.e., when something is added/removed/accessed)? Also not necessarily in this PR, just an idea for the future.

Good point, I will think of that in a future PR

But notice, this is only executed when we actually have to serialize, which is properly going to dominate the performance anyways.

But what if you have to serialize many small objects? Maybe that can become a bottleneck at some point?

Right, that could be a potential problem.

dask_cuda/proxy_object.py

Co-authored-by: Peter Andreas Entschev <[email protected]>

madsbk · 2021-09-08T06:37:26Z

Overall looks good, great work @madsbk . I have only a few minor comments/suggestions.

Thanks @pentschev for the review, I think I have addressed all your suggestions.

pentschev

LGTM, thanks @madsbk !

pentschev · 2021-09-08T06:40:48Z

@gpucibot merge

github-actions bot added the python python code needed label Aug 26, 2021

madsbk added 2 - In Progress Currently a work in progress improvement Improvement / enhancement to an existing function non-breaking Non-breaking change and removed python python code needed labels Aug 26, 2021

github-actions bot added the python python code needed label Aug 26, 2021

madsbk changed the title ~~Simplify proxy tracking~~ [WIP] Simplify proxy tracking Aug 26, 2021

madsbk force-pushed the simplify_proxy_tracking branch 2 times, most recently from 6e9079d to 11e36c1 Compare August 27, 2021 11:57

madsbk added 16 commits September 6, 2021 09:48

clean up

ce4a260

mypy: type hints

617a91a

ProxyObject: clean up

dab3964

DeviceHostFile: clean up local_directory argument

c932100

clean up

ed19cb8

Clean up

e7975d1

More type hints

716a179

Disallowing proxy of collections

660ca0f

In order to address ambiguity when serializing using multiple serializers, we disallow proxies of collections such as list or tuples. Instead, users should wrap each collection item in a proxy.

Now tracking a proxy's actual serializer

871ac97

Re-implemented proxy tracking

d98db98

get_device_memory_objects(): now incl. serialized data

1406076

ProxyObject: call the new finalizer

d8136dc

proxify(): remove the external finalizer

704037f

Fixed _mem_usage typo

252e717

Tracking now passes all tests

2d7f030

Some docs

531932f

madsbk force-pushed the simplify_proxy_tracking branch from 11e36c1 to 531932f Compare September 6, 2021 07:57

madsbk added 3 commits September 6, 2021 17:04

Minor clean up

7d5ab18

minor doc clean up

c15c394

Proxies now use use the manager's lock

4cc1a3e

madsbk added 4 commits September 7, 2021 09:39

Docs and clean up

19587c3

mem_usage_add(): proxy can have an empty set of dev buffers

3b614fe

Proxies.remove(): raise warning when the tally isn't going to zero

a70e664

now using the proxy's dtor instead of a finalizer

451d752

madsbk changed the title ~~[WIP] Simplify proxy tracking~~ Fix deadlock and simplify proxy tracking Sep 7, 2021

madsbk marked this pull request as ready for review September 7, 2021 11:26

madsbk requested a review from a team as a code owner September 7, 2021 11:26

madsbk added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Sep 7, 2021

madsbk added 2 commits September 7, 2021 16:07

Implements get_proxies_by_serializer()

23c8b4b

removed evict()

a4fdb60

pentschev reviewed Sep 7, 2021

View reviewed changes

madsbk and others added 2 commits September 8, 2021 08:23

Clean up by @pentschev

5c4ca1a

Co-authored-by: Peter Andreas Entschev <[email protected]>

Renamed mem_usage_sub => mem_usage_remove

4225364

pentschev approved these changes Sep 8, 2021

View reviewed changes

rapids-bot bot merged commit 33e5d3e into rapidsai:branch-21.10 Sep 8, 2021

madsbk deleted the simplify_proxy_tracking branch September 17, 2021 06:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix deadlock and simplify proxy tracking #712

Fix deadlock and simplify proxy tracking #712

madsbk commented Aug 26, 2021 •

edited

Loading

codecov-commenter commented Aug 27, 2021 •

edited

Loading

pentschev left a comment

pentschev Sep 7, 2021

madsbk Sep 8, 2021

madsbk Sep 8, 2021 •

edited

Loading

pentschev Sep 8, 2021

madsbk Sep 8, 2021

madsbk commented Sep 8, 2021

pentschev left a comment

pentschev commented Sep 8, 2021

Fix deadlock and simplify proxy tracking #712

Fix deadlock and simplify proxy tracking #712

Conversation

madsbk commented Aug 26, 2021 • edited Loading

codecov-commenter commented Aug 27, 2021 • edited Loading

Codecov Report

pentschev left a comment

Choose a reason for hiding this comment

pentschev Sep 7, 2021

Choose a reason for hiding this comment

madsbk Sep 8, 2021

Choose a reason for hiding this comment

madsbk Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

pentschev Sep 8, 2021

Choose a reason for hiding this comment

madsbk Sep 8, 2021

Choose a reason for hiding this comment

madsbk commented Sep 8, 2021

pentschev left a comment

Choose a reason for hiding this comment

pentschev commented Sep 8, 2021

madsbk commented Aug 26, 2021 •

edited

Loading

codecov-commenter commented Aug 27, 2021 •

edited

Loading

madsbk Sep 8, 2021 •

edited

Loading