Bug 56338 – Introduce NonThreadSpecific template

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Nat Duca]

Introduce NonThreadSpecific template

[Nat Duca]

Created attachment 85725 [details]
Patch

[Nat Duca]

Created attachment 85730 [details]
Patch

[Alexey Proskuryakov]

What are the use cases for this? Generally, no data should be shared between threads - is the idea to wrap every data member in every class in NonThreadSafe?

[Nat Duca]

(In reply to comment #3)
> What are the use cases for this? Generally, no data should be shared between threads - is the idea to wrap every data member in every class in NonThreadSafe?

The use case we had (for the chrome compositor) was the following:

 ViewManager {
   struct CompositorState {
   }
   NonThreadSafe<CompositorState> m_compositorState;
 };

[Alexey Proskuryakov]

Comment on attachment 85730 [details]
Patch

I don't think that this would be useful in WebKit. We strongly prefer message passing to having any shared data, so no assertions beyond ASSERT(isMainThread()) in a few methods should be necessary in practice.

All data is single threaded by default, and as long as we stick to that at design stage, runtime assertions are a weak and unnecessary way to guarantee the same.

[James Robinson]

OK, we'll keep this in chromium-specific directories and not contribute it to the project as a whole if you prefer.

[Nat Duca]

(In reply to comment #5)
> All data is single threaded by default, and as long as we stick to that at design stage, runtime assertions are a weak and unnecessary way to guarantee the same.

Thanks for the feedback. Here's a template of what I'd like to get done... can you help suggest a way that would be webkitty to get this done? The more detailed use case is here:
   https://bugs.webkit.org/show_bug.cgi?id=56131

Snippet form:
class ViewManager {
   static ViewManager* getInstance();

   class Message {
     virtual void Run() = 0;
   }

   CompositorThreadState {
     ... stuff specific to the compositor thread about views...
   }

   static void run() {
     while(true) {
       MessageQueue* mq = m_mq.waitForMessage();
       if(!mq) return;
  
     }
   }
   MessageQueue<Message> m_mq;
}

Now, in this case, I really can't take your advice and put everything on the main thread. So, suggestions?

[Nat Duca]

Bleh, replied to soon.. my apologies:

> class ViewManager {
>    // Only one ViewManager, but it doesn't actually own the thread, because
>    // its not the only thing doing work on the compositor thread.
>    // 
>    static ViewManager* getInstance();
>    // Posts message to the compositor thread to add the view
>    void AddView(..);
>    // Posts message to remove the view
>    void RemoveView();
>
>    // Active views on the compositor side...
>    CompositorThreadState {
>       // impls that the compositor is drawing
>       hashmap<View*,ViewImpl*> viewToImpl;
>    }
>    // Active views on the main thread
>    MainThreadState {
>       vector<View*> views;
>    }
> }
> 
>

Now, if the viewmanager owns the compositor thread, I could imagine making CompositorThreadState a local variable on the run function. Perhaps thats where your head was at?

The way we factored the code, we actually plan to run a few things on this second thread, meaning that the CompositorThread doesn't actually know about ViewManager explicitly. Rather, compositorThread is just a general message pump:
> // Owns the thread and the message pump, but does more than just process
> // ViewManager code
> class CCThread {
>   // only one CCThread
>   static CCThread* getInstance();
>   static void run() {
>      while(true) {
>        MessageQueue* mq = m_mq.waitForMessage();
>        if(!mq) return;
>           mq->run();
>      }
>    }
>    class Message {
>      virtual void Run() = 0;
>    }
>    MessageQueue<Message> m_mq;
> }

So the problem here is that I don't have a clearcut place to put this compositor thread state.

Any suggestions, now that I've been a bit more articulate?

[Alexey Proskuryakov]

From what I can tell from the code snippets, the design appears sound, and the only things that are needed is better naming and better encapsulation. With that, the risk of accidentally using a main thread data object from secondary thread would be negligible.

The class name itself raises a red flag. Instead of trying to explain it in my own words, I'll just give some links I found with a quick web search: <http://www.codinghorror.com/blog/2006/03/i-shall-call-it-somethingmanager.html>, <http://c2.com/cgi/wiki?DontNameClassesObjectManagerHandlerOrData>, <http://stackoverflow.com/questions/562971/smelly-class-names>.

CompositorThreadState is not something that should exist in a main thread object. Threads run concurrently, so there is no reliable way to track a thread state in ViewManager (unless you synchronize every access to the state with a mutex, which is not what we're trying to do). Instead of trying to track thread state, ViewManager should have its own state that's updated by messages sent from the secondary thread.

There are many examples of deferring work to secondary threads in WebCore, although some are not so good design, and some are more complicated that may bee needed here. For example, worker threads seem similar, although the design is complicated by the need to support both multi-thread and multi-process execution models.

[Nat Duca]

(In reply to comment #9)
> From what I can tell from the code snippets, the design appears sound, and the only things that are needed is better naming and better encapsulation. With that, the risk of accidentally using a main thread data object from secondary thread would be negligible.

Sadly, I'm really struggling to get a concrete next steps from your commentary... I'm hearing and symphathizing with your "bad smell" commentary, though!

> The class name itself raises a red flag.
Noted, but respectfully disagree. s/Manager/Collection/ if it makes you happier, the overall point remains the same.

> CompositorThreadState is not something that should exist in a main thread ...

I think I'm hearing you say:
  CCViewCollection { // Main thread object
     void AddView(); // needs to send an AddView message to CCViewImplCollection
     ... other stuff on the view collection...
     void RemoveView();
     vector<CCView> m_views;
  }

  CCViewImplCollection { // CCThread-only usage
    HashMap<CCView*, CCViewImpl> m_impls;
  }

But here's the question, who owns the CCViewImplCollection? I could make it an OwnPtr on CCViewCollection:
  class CCViewCollection {
    OwnPtr<CCViewImplCollection> m_viewImplCollection;
  }

But then some person new to the codebase could inadvertently reach into CCViewImpl and poke around on it! This is *exactly* the situation I'm trying to prevent. I've seen this happen time and time again...

So, suggestions?

[Alexey Proskuryakov]

I've CC'ed a few chromium folks who work on threading code a lot, and who could provide guidance.

[Dmitry Titov]

I agree with Alexey, this is a dangerous pattern, and we all wish it was not used. There is a difference between wish and reality though - so we do have some of that already, although this arguably increases maintenance costs and causes flakiness. I can provide some observations and a suggestion for your consideration. Please feel free to use them or not, but please treat threading code with utmost respect, not unlike a loaded gun :-)

Observations:

1. The design clearly assumes access to View objects from both threads. Initially when those objects are simple it may be fine (easy to trace all usages to figure out there is no funny business going on) but as people add stuff to it (even simple strings), the code becomes unanalyzable.

2. ViewManager is itself used from 2 threads. The design assumes there is a part of it that lives on main thread and a part that lives on Compositor thread. Same issue, with time parts become intermixed, call each other, it becomes a mess. A mess in treading scenario is seen as 'flakiness' and is very hard to trace down.

3. We do have a stream of issues related to using objects on more then one thread, most notorious are SQL Database (see bug 55919 for example) and Workers (see bug 48996 for example). They are hard to find and are always caused by a perfectly reasonable and expected inability of engineers to imagine all the control sequences in multithreaded environment. It's a good practice to try to avoid sharing data structures across threads for that reason.

Suggestion:

In your case, there can be a few design choices. One of them could be to have 3 separate objects:
- the ViewManagerProxy, which lives on both threads, has 2 sets of methods (with asserts IsMainThread()/IsCompositorThread() as approriate) and pointers to ViewManager and ViewManagerImpl , no other data likely.
- the ViewManager that lives on main thread and keeps a ptr to ViewManagerProxy but not ViewManagerImpl
- the ViewManagerImpl that lives completely on Compositor thread and keeps ptr to ViewManagerProxy but not to ViewManager

The ViewManagerProxy would make copy of all data going cross-thread and use tasks to post those copied bits cross-thread.  Basically, ViewManagerProxy would do nothing except passing data, then it is easier to review its code - no other ViewManager specifics. Ideally, we would have a generic class for that. It also initially creates ViewManager and ViewManagerImpl and manages their lifetime.

Other classes, like View, should not travel over thread boundary w/o being copied/serialized in the proxy. It's always better to have mirroring collections of objects on both sides of threading with matching Ids then share the same collection of objects cross-thread with synchronization.

Something like that. The idea is to make the code easier to review by separating pieces that work on separate threads as much as possible and keeping the part that actually is cross-tread simple and 'mechanical'. This is true not only about initial review, but all subsequent ones.

Hope this explains a bit the reluctance to add shared objects :-)

[David Levin]

"My TL;DR; summary" You may need some shared state across threads. May it as small as possible. (Then go back and make it smaller.) Using message passing. Feel free to ask for help with the design.

Dmitry does a more thorough job of answering.

[Nat Duca]

Thanks for all the feedback.

The good news is that the the actual threaded compositor code maintains CCView and CCViewImpl as webkit-only and compositor-only concepts. The point of NonThreadSafe<T> was to simplify writing the proxy class, not to enable some sort of mutable shared state between threads.

The NonThreadSafe helper class is just for cleaning up threaded code. Using Dmitry's suggested proxy as a starting point:
   CCViewManagerProxy {
   protected:
      CCViewManagerProxy(ThreadHandle implThread) : m_implThread(implThread) { }
      CCViewManager& viewManager() {
         ASSERT(isMainThread();
         return m_viewManager;
      }
      CCViewManagerImpl& viewManagerImpl() {
         ASSERT(currentThread() == m_implThread);
         return m_viewManagerImpl;
      }
   private:
      ThreadHandle m_implThread;
      CCViewManager m_viewManager;
      CCViewManagerImpl m_viewManagerImpl;
   }

Having NonThreadSafe<T> lets us turns this into:
   CCViewManagerProxy {
     CCViewManagerProxy(ThreadHandle implThread) { m_viewManagerImpl.setValid(implThread); }
     CCViewManager& viewManager() { return m_viewManager.get(); }
     CCViewManagerImpl& viewManagerImpl() { return m_viewManagerImpl.get(); }
   private:
     NonThreadSafe<CCViewManager> m_viewManager;
     NonThreadSafe<CCViewManagerImpl> m_viewManagerImpl;
   }

The nice thing here is, in addition to brevity, I can now add methods to CCViewManagerProxy and be confident that the stateful fields remain sealed.

Anyways, good disucssion. Thanks, and given response here, I think I'll close this bug.


- N