ARROW-2195: [Plasma] Return auto-releasing buffers by pitrou · Pull Request #1807 · apache/arrow

pitrou · 2018-03-29T09:56:59Z

On the C++ side, add a new PlasmaClient::GetAuto() method to return buffers that release
the corresponding object on destruction.

On the Python side, return such buffers in PlasmaClient.get_buffers().

pitrou · 2018-03-29T12:02:49Z

This changes/adds some APIs on the CUDA side so will need reviewing from someone familiar with that code base.

pitrou · 2018-04-02T10:21:10Z

@robertnishihara @pcmoritz @Wapaul1 Is one of you interested in reviewing this?

pcmoritz · 2018-04-02T17:38:03Z

cpp/src/plasma/common.h

Let's put this into https://github.com/apache/arrow/blob/master/cpp/src/arrow/util/macros.h. It seems more broadly useful for the whole arrow project.

pcmoritz

Thanks for doing this! On a high level, here is what we should attempt: Make it so the new APIs CreateAuto and GetAuto can be used as replacements for Create and Get and then deprecate the old ones and rename CreateAuto -> Create and GetAuto -> Get. I can give some more lower level comments about that in the PR.

pcmoritz · 2018-04-02T18:19:19Z

cpp/src/plasma/client.cc

we have ARROW_UNUSED for this:

arrow/cpp/src/arrow/util/macros.h

Line 28 in ecb7605

#define ARROW_UNUSED(x) (void)x

pcmoritz · 2018-04-02T18:23:57Z

cpp/src/plasma/client.cc

This should be exposed through the Info method, see

arrow/cpp/src/plasma/client.h

Line 301 in ecb7605

Status Info(const ObjectID& object_id, int* object_status);

Given that is is private, we can leave it for now, but can you create a JIRA for it?

Ok, see ARROW-2379

Info requires an IPC to the plasma store and seems too heavy-weight since this can all be determined with locally available information.

IsInUse is probably better named something like IsInUseByThisClient (that's an ugly name, but I think it's more clear).

We have a lot of code in client.cc that's already doing stuff like

auto elem = objects_in_use_.find(object_id); if (elem == objects_in_use_.end()) { ... }

so it may make sense for IsInUse to replace some of that code.

pcmoritz · 2018-04-02T18:26:54Z

cpp/src/plasma/client.cc

Can we get rid of this callback, and do the wrapping into PlasmaBuffer as a post-processing step to GetBuffers?

That sounds a bit brittle. You want to have a single PlasmaBuffer per exported (e.g. mmap'ed) piece of memory, but both data and metadata share that piece of memory.

pcmoritz · 2018-04-02T18:39:48Z

How do people feel about replacing the old Get and Create methods with new ones which return PlasmaBuffers, and make Release private (which means PlasmaBuffer needs to be friend of PlasmaClient). Given that 0.9 has just been released it might be the right time to do it.

pitrou · 2018-04-02T18:46:50Z

I suppose that depends how you feel about breaking compatibility without notice :-)

pcmoritz · 2018-04-02T21:01:14Z

Ok, let's do the following:

Deprecate the old Create and Get and Release
Make new ones with the following signature:

// sizes are already available through buffers, no need to store them separately
struct PlasmaObject {
  std::shared_ptr<Buffer> data;
  std::shared_ptr<Buffer> metadata;
  int device_num;
}

Status Get(const std::vector<ObjectID>& object_ids, int64_t timeout_ms, std::vector<PlasmaObject>* objects);

Status Create(const ObjectID& object_id, int64_t data_size, int64_t metadata_size, PlasmaObject* object);

// (the metadata would be written to shared memory directly by the client instead of passing it in)

which are backed by PlasmaBuffers and don't need explicit release

I'm slightly uncomfortable that the two overloaded ones have different behavior and the same name, but going forward that seems to be the best solution (we should try to avoid having two rounds of deprecations).

robertnishihara · 2018-04-02T22:33:55Z

cpp/src/plasma/client.h

It's ok for it to be sealed before it is released.

Maybe "The returned object must be released once it is done with. It must also be either sealed or aborted."?

Indeed, it seems you're right.

robertnishihara · 2018-04-02T22:41:32Z

python/pyarrow/plasma.pyx

Should this be changed to use GetAuto?

Yes, I had overlooked that. Thank you.

robertnishihara · 2018-04-02T22:44:43Z

cpp/src/plasma/client.cc

Does this mean we can get rid of

arrow/python/pyarrow/plasma.pyx

Line 182 in 7e27cf5

cdef class PlasmaBuffer(Buffer):

now?

Once we migrate Create() too, yes.

On the C++ side, add a new PlasmaClient::GetAuto() method to return buffers that release the corresponding object on destruction. On the Python side, return such buffers in PlasmaClient.get_buffers().

pitrou · 2018-04-03T11:04:14Z

@pcmoritz I would suggest tackling Create() in a separate PR, since there are additional complications in how it interacts with Seal() and Abort(). What do you think?

pitrou · 2018-04-03T12:23:42Z

Note an ideal API would have the Seal(), Abort() and Release() methods on the PlasmaBuffer, not on PlasmaClient.

pcmoritz · 2018-04-03T19:08:20Z

cpp/src/arrow/util/macros.h


+// ----------------------------------------------------------------------
+// From googletest
+// XXX also in parquet-cpp


remove the XXX

pcmoritz · 2018-04-03T19:18:15Z

cpp/src/plasma/test/client_tests.cc

    ARROW_CHECK_OK(client2_.Disconnect());
-    system("killall plasma_store &");
+    // Kill all plasma_store processes
+    // XXX should only kill the processes we launched


replace XXX by TODO

pcmoritz · 2018-04-03T19:20:51Z

Awesome, this is really great! Let's remove the XXX in the PR and then we can merge it.

robertnishihara · 2018-04-03T19:53:59Z

Nice job @pitrou! This will be a lot cleaner. Especially once we can do the same thing for Create and then remove the old code paths.

pcmoritz · 2018-04-03T20:03:01Z

I also created a JIRA for the Create API (very similar to what you propose, in addition it allows us to get rid of the Abort() method): https://issues.apache.org/jira/browse/ARROW-2386

pitrou · 2018-04-04T17:25:51Z

@pcmoritz I fixed the XXX's.

robertnishihara · 2018-04-11T21:04:17Z

@pitrou I'm seeing https://issues.apache.org/jira/browse/ARROW-2448 when using this PR.

It seems like we need each client to keep track of the buffers that it produces and to invalidate them when the client disconnects (or something like that). cc @pcmoritz

pitrou force-pushed the ARROW-2195-plasma-buffers branch 2 times, most recently from 43d488f to e1a22da Compare March 29, 2018 14:05

pitrou changed the title ~~[WIP] ARROW-2195: [Plasma] Return auto-releasing buffers~~ ARROW-2195: [Plasma] Return auto-releasing buffers Mar 29, 2018

pitrou force-pushed the ARROW-2195-plasma-buffers branch from 8a01b22 to c0db264 Compare April 2, 2018 10:39

pcmoritz reviewed Apr 2, 2018

View reviewed changes

robertnishihara reviewed Apr 2, 2018

View reviewed changes

pitrou added 9 commits April 3, 2018 10:10

ARROW-2195: [Plasma] Return auto-releasing buffers

c2af4d3

On the C++ side, add a new PlasmaClient::GetAuto() method to return buffers that release the corresponding object on destruction. On the Python side, return such buffers in PlasmaClient.get_buffers().

Allow getting back CUDA buffer from buffer

483bbdb

Cleanups

30a439d

Add Python test

dee2f5d

Add tests for device_num

43b377f

Try to fix odd failure

123ea83

Move FRIEND_TEST to the Arrow codebase

4d33403

Use ARROW_UNUSED()

562abc5

Use GetAuto() in plasma.pyx, not Get()

927a535

pitrou force-pushed the ARROW-2195-plasma-buffers branch 2 times, most recently from 167dc0c to e869757 Compare April 3, 2018 11:03

pitrou force-pushed the ARROW-2195-plasma-buffers branch from e869757 to 04c1842 Compare April 3, 2018 11:27

Migrate PlasmaClient::Get()

ac2d84c

pitrou force-pushed the ARROW-2195-plasma-buffers branch from 04c1842 to ac2d84c Compare April 3, 2018 11:30

pcmoritz reviewed Apr 3, 2018

View reviewed changes

Remove XXX comments

e3747cf

pcmoritz closed this in cf39686 Apr 4, 2018

pitrou deleted the ARROW-2195-plasma-buffers branch April 4, 2018 17:55

robertnishihara mentioned this pull request Apr 11, 2018

Update arrow and parquet-cpp. ray-project/ray#1875

Merged

pcmoritz mentioned this pull request Apr 24, 2018

Incorporate C++ Buffer management and Seal global threadpool fix from arrow ray-project/ray#1950

Merged

This was referenced Apr 11, 2018

[Plasma] Segfault when retrieving RecordBatch from plasma store #18159

Closed

[Plasma] Change PlasmaClient::Create API #18420

Closed

Conversation

pitrou commented Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou commented Mar 29, 2018

Uh oh!

pitrou commented Apr 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pcmoritz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pcmoritz commented Apr 2, 2018

Uh oh!

pitrou commented Apr 2, 2018

Uh oh!

pcmoritz commented Apr 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou commented Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou commented Apr 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pcmoritz commented Apr 3, 2018

Uh oh!

robertnishihara commented Apr 3, 2018

Uh oh!

pcmoritz commented Apr 3, 2018

Uh oh!

pitrou commented Apr 4, 2018

Uh oh!

robertnishihara commented Apr 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pitrou commented Mar 29, 2018 •

edited

Loading

pcmoritz commented Apr 2, 2018 •

edited

Loading

pitrou commented Apr 3, 2018 •

edited

Loading