Page MenuHomePhabricator

[ORC] Fix the move-captured std::unique_ptr vs. std::function dilemma
AbandonedPublic

Authored by sgraenitz on Jan 6 2020, 1:16 PM.

Details

Summary

C++14 is there for a while now and the FIXME is getting confusing. There was an earlier attempt to fix this the way (I think) it was indended originally: https://github.com/llvm/llvm-project/commit/ce74c3b19f5b#diff-f27b9f96303108227f0119d85d6c3ddf (It was rolled back here: https://github.com/llvm/llvm-project/commit/6baaa4be7831#diff-f27b9f96303108227f0119d85d6c3ddf )

  ES->setDispatchMaterialization(
      [this](JITDylib &JD, std::unique_ptr<MaterializationUnit> MU) {
+       auto Work = [MU = std::move(MU), &JD] { MU->doMaterialize(JD); };
-       // FIXME: Switch to move capture once we have c++14.
-       auto SharedMU = std::shared_ptr<MaterializationUnit>(std::move(MU));
-       auto Work = [SharedMU, &JD]() { SharedMU->doMaterialize(JD); };
        CompileThreads->async(std::move(Work));
      });

Unfortunately this doesn't work, because the lambda that captures the std::unique_ptr is being passed to ThreadPool::async() as a std::function: Holding the std::unique_ptr its copy constructor is deleted implicitly, but the standard requires the value of std::function to be copy-constructible.

What's the options? We certainly want to use ThreadPool, so we are bound to std::function. I think we don't want to go through a raw pointer capture by-value here, especially seeing that we pass a std::function next..

The current workaround creates and copies a std::shared_pointer. That's quite expensive. In a first step we can move-capture it in the Work lambda to avoid the copy. I think we don't get around the creation (assuming we keep a smart pointer), so why not receive it as a std::shared_ptr directly? The only disproportionate overhead I see is in the materializeOnCurrentThread case. I changed that to not calling the function at all.

What do you think?

Event Timeline

sgraenitz created this revision.Jan 6 2020, 1:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2020, 1:16 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

The current workaround creates and copies a std::shared_pointer. That's quite expensive. In a first step we can move-capture it in the Work lambda to avoid the copy. I think we don't get around the creation (assuming we keep a smart pointer), so why not receive it as a std::shared_ptr directly? The only disproportionate overhead I see is in the materializeOnCurrentThread case. I changed that to not calling the function at all.

In the context of running a Materializer I don’t think we need to be worried about the shared_ptr construction overhead, but it would still be nice to tidy this up.

For our purposes it would be ideal if ThreadPool::async took an llvm::unique_function rather than a std::function. I wonder if that would work for other clients too? I think 99% of ThreadPool use cases would work equally well or better with a unique_function, and it looks like unique_function should be constructible from a std::function (with minor runtime overhead for execution) for the 1% of cases that are not supported.

sgraenitz abandoned this revision.Sat, Jan 18, 8:43 AM

Indeed, that sounds like a better solution.