[PATCH] Speculatively instantiate archive members

Rui Ueyama ruiu at google.com
Fri Jan 16 14:42:18 PST 2015


================
Comment at: lib/ReaderWriter/FileArchive.cpp:274
@@ +273,3 @@
+  mutable std::map<const char *, std::future<const File *>> _preloaded;
+  mutable std::vector<std::unique_ptr<std::promise<const File *>>> _promises;
+  mutable std::mutex _mutex;
----------------
ruiu wrote:
> atanasyan wrote:
> > ruiu wrote:
> > > atanasyan wrote:
> > > > Can we store promises directly and do not use `unique_ptr` wrapper? Though I did not test the following code, it compiled successfully. Does it have a sense?
> > > > 
> > > > ```
> > > > // Instantiate the member
> > > > _promises.emplace_back();
> > > > auto &promise = _promises.back();
> > > > _preloaded[memberStart] = promise.get_future();
> > > >                                                
> > > > group.spawn([this, &promise, ci] {
> > > >   std::unique_ptr<File> result;
> > > >   if (instantiateMember(ci, result)) {
> > > >     promise.set_value(nullptr);
> > > >     return;
> > > >   }
> > > >   promise.set_value(result.release());
> > > > });
> > > > 
> > > > [...]
> > > > 
> > > > mutable std::vector<std::promise<const File *>> _promises;
> > > > ```
> > > I think promises created here need to be deleted when it's no longer needed. Your code wouldn't delete promises, no?
> > They will be destroyed at the same time with the `_promises` container as any other types own destructor and stored in the `std::vector`.
> I made a change as you suggested but eventually decided to roll it back because it didn't work on Windows. I don't fully understand why it sometimes raised an exception -- there might be a bug in std::future's move constructor? Anyways, the cost of referencing futures through unique_ptr should be negligible here.
I think I understand the reason. A future returned by promise::get_future() has a reference to the promise. When a vector is extended and all elements are moved, the original promise becomes invalid (whose valid() would return false.) When that happens, future::get() fails with an exception.

http://reviews.llvm.org/D7015

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list