Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Convert DefaultFileSource and dependents to actor model #7678

Closed
wants to merge 5 commits into from

Conversation

kkaefer
Copy link
Contributor

@kkaefer kkaefer commented Jan 11, 2017

Getting started on #6426. This is still a work in progress.

A few large changes:

  • the DefaultFileSource constructor now takes a Scheduler
  • the AssetFileSource does not use its own thread anymore; instead it uses the shared worker threads

Given that our file requests need a way to "cancel" them (i.e. have the callback not invoked, I introduced AssetFileSource::Impl::Request, which holds a shared_ptr to the callback. If that object gets destroyed, the weak_ptr we're passing to the worker thread will be empty, and back on the main thread, the callback obviously can't be fired anymore. I'm not sure if using this violates @jfirebaugh's plea about not using std::shared_ptr with actors (since technically, you could try to lock the weak_ptr in the worker.

I also added a MGLThreadPool object for Darwin, but implemented a global singleton for Android . I believe we should go the global singleton instance for Darwin as well (and remove MGLThreadPool again), since Darwin never deallocates the DefaultFileSource as part of the regular application workflow anyway (it's a singleton in MGLOfflineStorage).

@kkaefer kkaefer added refactor ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Jan 11, 2017
@mention-bot
Copy link

@kkaefer, thanks for your PR! By analyzing this pull request, we identified @jfirebaugh, @tmpsantos and @1ec5 to be potential reviewers.

@kkaefer
Copy link
Contributor Author

kkaefer commented Jan 11, 2017

The long term goal is to move the other FileSources onto the actor model as well, which enables us to remove util::Thread.

@tmpsantos
Copy link
Contributor

I gave this a shot once and I got stuck. The main problem I faced when removing Thread from the DefaultFileSource was that we need a RunLoop for some platforms on the worker thread for pooling for events. Create a new one for each new request is probably no viable. Also the HTTPFileSource::Impl is usually not thread safe and lives in a fixed thread.

We probably need a new scheduler that is not ThreadPool for the DefaultFileSource at least in some platforms we support.

#pragma mark - Internal

+ (instancetype)sharedPool {
if (NSProcessInfo.processInfo.mgl_isInterfaceBuilderDesignablesAgent) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unnecessary, since there won't be an offline storage object if we're rendering for IB, and even if we were, spinning up this thread pool should be cheap. The reason we do this in MGLOfflineStorage and MGLAccountManager is to avoid potentially expensive operations that may block MGLMapView from rendering within the allotted time.

@interface MGLThreadPool : NSObject
@end

@interface MGLThreadPool (Private)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for a class extension, since the whole header is already internal to the project. A class extension would need to go in a separate header from the public header anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks; as per the comment above, I'm planning on removing MGLThreadPool altogether, since it's a singleton anyway.

@kkaefer
Copy link
Contributor Author

kkaefer commented Jan 23, 2017

@jfirebaugh given that you're most familiar with this system, could you please do a quick review of whether I'm moving into the right direction with this?

@jfirebaugh
Copy link
Contributor

The way I would model this is to make the request and response callback both first-class actors:

class RequestCallback {
public:
    // This class exists only to accept and ignore the "self" argument. Maybe it should be optional?
    RequestCallback(ActorRef<RequestCallback>, std::function<void (Response)>);
    void operator() (Response response) { callback(response); }

private:
    std::function<void (Response)> callback;
};

class AssetRequest {
public:
    AssetRequest(ActorRef<AssetRequest>);
    ~AssetRequest(); // cancels

    void send(const Resource& resource, ActorRef<RequestCallback> callback) {
        // do whatever, then eventually:
        callback.invoke(&RequestCallback::operator(), response);
    }
};

class SomeClass {
public:
    SomeClass(Scheduler& requestScheduler)
      : request(requestScheduler) {}

private:
    void requestingMethod() {
        request.invoke(&AssetRequest::send, resource, callback);
    }

    void receivingMethod(Response);

    Actor<AssetRequest> request;
    Actor<RequestCallback> callback {
        *util::RunLoop::Get(),
        std::bind(&SomeClass::receivingMethod, this)
    };
};

@ivovandongen ivovandongen mentioned this pull request Apr 25, 2017
28 tasks
@tmpsantos tmpsantos mentioned this pull request Apr 27, 2017
4 tasks
@kkaefer kkaefer added the Core The cross-platform C++ core, aka mbgl label May 30, 2017
@jfirebaugh
Copy link
Contributor

Stale.

@jfirebaugh jfirebaugh closed this Oct 24, 2017
@jfirebaugh jfirebaugh deleted the filesource-actor branch October 24, 2017 20:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants