[Webkit-unassigned] [Bug 63531] Turn PreloadScanner implementation to a thread based solution

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=63531





--- Comment #47 from Filip Pizlo <fpizlo at apple.com>  2013-01-10 15:19:58 PST ---
(In reply to comment #46)
> (In reply to comment #44)
> > (From update of attachment 103346 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=103346&action=review
> > 
> > > Source/WebCore/html/parser/HTMLPreloadScannerThread.cpp:58
> > > +    while (OwnPtr<HTMLPreloadScannerThread::ScanTask> task = m_tasks.waitForMessage()) {
> > > +        Mutex& scanTasksMutex = task->preloadScanner->scanTasksMutex();
> > > +        if (!scanTasksMutex.tryLock())
> > > +            continue;
> > > +        scan(task.get());
> > > +        scanTasksMutex.unlock();
> > > +    }
> > 
> > This seems like a really bad idea.  This looks like it will remove a task from the queue, but then if it cannot acquire the scanTasksMutex, it will just drop the task on the floor.
> 
> This condition can only be true if the parser is stopped or it is under destruction, so we don't want to process this task anyway.

Ah.  I buy it.

> 
> > 
> > > Source/WebCore/html/parser/HTMLPreloadScannerThread.h:36
> > > +    MessageQueue<ScanTask> m_tasks;
> > 
> > I took a peek into MessageQueue, and it appears not to be particularly optimal.  It pretty much requires everything happening in lockstep: to add a task, you lock a lock, add things, signal, and unlock.  When you do this and the worker thread was asleep, you pay the price of it being awoken.  That's expensive.  The locking itself is probably expensive, but that's not nearly as bad.
> 
> I'm not sure how could we avoid that. Do you mean we should make sure that the thread will never fall asleep, i.e. it always have something to do? This seems tricky, I don't have any idea how to achieve that.

It is tricky, and I'm also not sure how to achieve it.  This is basically always the problem in trying to get a speed-up from making something concurrent: you need to get enough concurrency to keep your threads always fed with work to do.

If we are sure that we cannot achieve this, then we should think twice about using concurrency as a performance optimization.  (Though it may still be a valid optimization for reducing worst-case latencies if we believe that the task we have made concurrent risks stalling, say because of I/O or because of an algorithmic pathology.  That doesn't appear to be the case here since you're just moving non-I/O linear-time work off the main thread.)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.