gloo icon indicating copy to clipboard operation
gloo copied to clipboard

uvw C++11 looks like a good idea

Open skypjack opened this issue 6 years ago • 3 comments

I saw this commit and I must admit that it's an honor that Facebook has decided to use uvw. Furthermore, I think that your idea of bringing it to C++11 may be of interest to many users. I was contacted by several people after switching to C++17, all of them asked to maintain a C++11/14 version, but I don't have enough bandwidth to do it myself right now since uvw is and has always been so far a free time project.

Do you think there is the possibility of a collaboration in this sense, given also your need? Having Facebook as a partner would surely be an excuse but, more than anything else, knowing that you have someone who can support me with the development would give new life to the project and allow to support a C++11 version without problems.

I'm looking forward to your answer. Thank you in any case.

skypjack avatar Aug 30 '19 11:08 skypjack

Hi @skypjack, thanks for creating the issue!

First of all, thanks for creating uvw. It's the top library that came up for using libuv from C++ and it's been a pleasure to read the code and to understand the patterns you applied. Given I only needed a subset here, it was easier to copy the bits that could be used verbatim (the event emitter), and reimplement the same pattern (of leaking and releasing a shared_ptr) for memory management, than it was to fork and make the whole library work with C++11.

I'd be happy to switch this out and make it use uvw directly if it would work with C++11. I'm thinking the most important thing to fix is the unfortunately lack of move semantics for lambda capture, but I'm sure there's other issues after that as well.

Aside from the C++11 compatibility, I also added some functionality to complete a full read (potentially over multiple uv_read_callback calls), and to allow for a read to work against a user specified chunk of memory. I'm not sure if it would be easy to port those to uvw, seeing as the user defined memory one would require changing the default behavior to pause when all chunks are read. Anyway, we can discuss details like that separately of course.

What would be your preferred way to continue?

pietern avatar Sep 09 '19 12:09 pietern

Hi @pietern and thank you for answering! Really appreciated.

What would be your preferred way to continue?

If you're interested in collaborating to port uvw to C++11, all the ways are more or less fine for me. We can continue here, by mail (available from my profile) or whatever you suggest that is feasible.

I'm thinking the most important thing to fix is the unfortunately lack of move semantics for lambda capture

I'm not sure I get what you mean here. I don't remember all the details of your changes, so probably I'm missing something important but why do you want to move what you put in a capture list? Isn't it possible anyway if you make the lambda mutable? You would move what's contained in a let's say data member (ok, it's not guaranteed to be such but I'm sure you get what I mean), it's fine as long as you don't use it anymore because after you moved the member it's usually in a safe but unspecified state.

Anyway, we can discuss details like that separately of course.

Definitely. This sounds more like a feature to add to the library that doesn't depend on the revision of the standard. I'd say there are no problems with it but I'd discuss it separately.

skypjack avatar Sep 09 '19 12:09 skypjack

Apologies for the lack of action on this issue, @skypjack. I haven't gotten around to working on this. In the mean time, PyTorch (where Gloo is predominantly used) is in the process of moving to C++14, which makes C++11 support less important here as well.

Regarding the capture-by-move issue: this is a C++14 and newer feature, so making uvw work for C++11 meant changing all usages of lambdas that capture by move. This is not hard and performance-wise only causes an additional refcount bump, since everything is a shared_ptr anyway.

pietern avatar Dec 11 '19 13:12 pietern