ARROW-4983: [Plasma] Unmap memory upon destruction of the PlasmaClient

[pcmoritz]

No description provided.

[guoyuhong]

This PR is used to fix: ray-project/ray#4385 (comment) .

[fsaintjacques]

Multiple issues here:

1. The object supports copy and assign, you probably don't want this (use ARROW_DISALLOW_COPY_AND_ASSIGN).
2. The fields are not initialized by default, make uint8_t *ptr = nullptr; and size_t length = 0.
3. What about the file descriptor?
4. mmap in one class and munmap in the destructor of another class, seems like bad design and an afterthought.

[fsaintjacques]

I also note that Client uses LookupOrMmap to create MutableBuffer in PlasmaClient::Impl::Create, all said buffers needs to be reclaimed too.

[pcmoritz]

@fsaintjacques Thanks! I fixed the issues you mentioned and made ClientMmapTableEntry into a C++ class that uses RAII to keep track of the mmaps/munmaps. For 3., the file descriptor is closed as soon as it is received from the store, in the constructor of ClientMmapTableEntry. The memory of MutableBuffers are reclaimed by the same mechanism, they also create ClientMmapTableEntrys that will do the munmaps on destruction (there are in general many buffers per memory mapped region).

[robertnishihara]

Why unique_ptr? If you do std::unordered_map<int, ClientMmapTableEntry>, then when you delete an entry from mmap_table_, its destructor will run.

[robertnishihara]

Oh do you need that in order to compile when using ARROW_DISALLOW_COPY_AND_ASSIGN?

[pcmoritz]

Yes, without the unique_ptr it would not be correct (upon rehashing). The constructor will run with unique_ptr as well.

[robertnishihara]

Suggested change

	: fd_(fd), pointer_(nullptr), length_(0) {
	: fd_(fd) {

there's no point to those initializations, right?

[robertnishihara]

Oh I see @fsaintjacques suggested this. That's fine I guess, but what's the reason for this?

[fsaintjacques]

Because if we remove the copy constructor, then move can swap in un-initialized values.

[fsaintjacques]

The problem with mutable buffer is that they do not extend the lifetime of the mmap-ed pointer. To change this, you could have a custom MutableBuffer that has a shared_ptr<ClientMmapTableEntry> (and also modify the Client class to use shared_ptr instead of unique_ptr`.

[pcmoritz]

@fsaintjacques I introduced PlasmaMutableBuffer to behave similar to the PlasmaBuffer class now, by holding a shared pointer to a PlasmaClient and keeping the client alive this way.

[robertnishihara]

Looks good to me!

[fsaintjacques]

I know this is now merged, but I think it would have been better to link the Buffer to the mmap entry instead of the client, this way you get eager reclamation. Otherwise, one buffer holds all mmap.

[pcmoritz]

@fsaintjacques You are right, however since #3490, most buffers are allocated into a single memory mapped file [*], so it wouldn't help in practice. Unfortunately we still need the mechanism to support multiple memory mapped files since most malloc implementation won't be able to guarantee that there is actually only a single mmaped file, in practice it can happen that there is the large one and then another small one for example.

[*] This has two advantages: 1) it will use the whole memory available. Before, it had a doubling strategy that would lead to only half of the memory being used in the worst case and 2) it avoids that files are mapped and unmapped, which led to performance overheads.