[Webkit-unassigned] [Bug 95057] [chromium] Allow impl-thread deletion of only some resources

[bugzilla-daemon at webkit.org]

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





--- Comment #24 from Eric Penner <epenner at chromium.org>  2012-09-04 16:09:49 PST ---
(From update of attachment 161809)
View in context: https://bugs.webkit.org/attachment.cgi?id=161809&action=review

I'm just back from vacation. The other comments seems to address any races/dead-locks
I can think of but I'll take another pass to see if I can think of anything else.
I've added some other general comments for now.

When moved entirely to the the impl-thread, do you anticipate this will
still have value, or is this mainly a hold-over? There might be other threading
issues, but I don't see this in particular being a problem.

On that note, while I kind of like all the isImplThread() ASSERTs etc., previously
passing the ResourceProvider* was the means of indicating that resources could
allocated/destroyed. This implicitly indicated isImplThread() && isMainThreadBlocked()
but didn't enforce it explicitly (so that this class could be used on any thread, and be kept
as a single-threaded data-structure rather than being so coupled to other classes).

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:62
>      virtual void postAnimationEventsToMainThreadOnImplThread(PassOwnPtr<CCAnimationEventsVector>, double wallClockTime) = 0;

I'm really curious as to why we can post an event for animations above but not similarly do so for texture management below. Why can we not post tasks to achieve the end result here rather than resorting to adding a mutex?

I doubt this is the only case where this type of communication will be needed, and I'm hoping that adding a mutex will not become the norm for solving such problems. So I just want to be sure we have considered all other options here.

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.h:80
>      void reduceMemory(CCResourceProvider*);

I find the naming confusing here given:
- reduceMemory above must be called on the impl thread (with main thread blocked) since it takes CCResourceProvider*.
- reduceMemoryOnImplThread below doesn't reduce memory to the limit, but rather clears all memory.

I would recommend:
- renaming to clearAllMemoryOnImplThread
- reserve "onImplThread" only for functions that could occur when the main thread is not blocked (if the main thread is blocked it is more like "onBothThreads" or "duringCommit").

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.h:160
> +        void addBackingOnImplThread(CCPrioritizedTexture::Backing*);

See my above comment about "onImplThread".

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