Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1900423004:
[compositorworker] Register compositor proxies with proxy client (Closed)
|
Created:
4 years, 8 months ago by majidvp Modified:
4 years, 6 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, blink-reviews-bindings_chromium.org, blink-reviews-dom_chromium.org, blink-worker-reviews_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, falken, flackr, gavinp+loader_chromium.org, haraken, horo+watch_chromium.org, Nate Chapin, kinuko+worker_chromium.org, loading-reviews_chromium.org, rwlbuis, tyoshino+watch_chromium.org, Ian Vollick Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[compositorworker] Register compositor proxies with proxy client
Each compositor proxy registers itself with its proxy client when created.
This populated set is later used to update the backing mutable state for each
compositor proxy.
TEST=These machinery is tested in the follow-up CL https://codereview.chromium.org/2041193005/
BUG=430155
Committed: https://crrev.com/eb14272233cd88227efc37a31fabb9688ed831ba
Cr-Commit-Position: refs/heads/master@{#399502}
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : Rebase and use WTF containers #Patch Set 4 : rebase #Patch Set 5 : minor update #
Total comments: 9
Patch Set 6 : Address feedback #
Total comments: 5
Patch Set 7 : Fix layout test #
Total comments: 10
Patch Set 8 : Address feedback #Patch Set 9 : Address feedback #
Total comments: 5
Patch Set 10 : Add clarifying comments #Patch Set 11 : replace map of set with a set #
Total comments: 2
Patch Set 12 : Update comment #Dependent Patchsets: Messages
Total messages: 42 (13 generated)
Description was changed from ========== Intorduce CompositorProxyClient and fix tests BUG= ========== to ========== [compositorworker] Add CompositorProxyClient CompositoryProxyClient is responsible for registering and unregistering compositor proxies as they are created on the compositor worker. BUG=430155 ==========
Description was changed from ========== [compositorworker] Add CompositorProxyClient CompositoryProxyClient is responsible for registering and unregistering compositor proxies as they are created on the compositor worker. BUG=430155 ========== to ========== [compositorworker] Register compositor proxies with proxy client Each compositor proxy registers and unregisters itself with its client when it is created and destructed. BUG=430155 ==========
Description was changed from ========== [compositorworker] Register compositor proxies with proxy client Each compositor proxy registers and unregisters itself with its client when it is created and destructed. BUG=430155 ========== to ========== [compositorworker] Register compositor proxies with proxy client Each compositor proxy registers itself with its proxy client when created. This populated map is later used to update the backing mutable state for each compositor proxy. TEST=These machinery is tested in the follow-up CL https://codereview.chromium.org/2041193005/ BUG=430155 ==========
majidvp@chromium.org changed reviewers: + jbroman@chromium.org
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/1900423004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/CompositorProxy.cpp (right): https://codereview.chromium.org/1900423004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/CompositorProxy.cpp:156: disconnect(); You don't need to call disconnect() because disconnect() has already been called in the pre-finalizer. https://codereview.chromium.org/1900423004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp (right): https://codereview.chromium.org/1900423004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp:72: TRACE_EVENT2("compositor-worker,compositor-proxy", registerCompositorProxy should be cheap, so it won't make sense to add a trace event. https://codereview.chromium.org/1900423004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp:87: TRACE_EVENT2("compositor-worker,compositor-proxy", Ditto. https://codereview.chromium.org/1900423004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/CompositorProxyClientImpl.h (right): https://codereview.chromium.org/1900423004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/CompositorProxyClientImpl.h:49: typedef HeapHashMap<uint64_t, Member<ProxyVector>> ProxyMap; Can you use a HashMap/HashSet of WeakMembers? Then you don't need to call unregisterCompositorProxy because the weak processing will automatically remove the entries when the CompositorProxy objects get destructed.
https://codereview.chromium.org/1900423004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/CompositorProxy.cpp (right): https://codereview.chromium.org/1900423004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/CompositorProxy.cpp:119: WorkerClients* clients = toWorkerGlobalScope(context)->clients(); Won't this crash if you postMessage a CompositorProxy back to the main thread? Why is it safe to assume the execution context is a WorkerGlobalScope?
https://codereview.chromium.org/1900423004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/CompositorProxy.cpp (right): https://codereview.chromium.org/1900423004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/CompositorProxy.cpp:119: WorkerClients* clients = toWorkerGlobalScope(context)->clients(); On 2016/06/08 14:45:34, jbroman wrote: > Won't this crash if you postMessage a CompositorProxy back to the main thread? > Why is it safe to assume the execution context is a WorkerGlobalScope? Fixed. It is not safe and crashes! Modified one of existing tests to cover this. https://codereview.chromium.org/1900423004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/CompositorProxy.cpp:156: disconnect(); On 2016/06/08 00:10:36, haraken wrote: > > You don't need to call disconnect() because disconnect() has already been called > in the pre-finalizer. Done. https://codereview.chromium.org/1900423004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp (right): https://codereview.chromium.org/1900423004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp:72: TRACE_EVENT2("compositor-worker,compositor-proxy", On 2016/06/08 00:10:36, haraken wrote: > > registerCompositorProxy should be cheap, so it won't make sense to add a trace > event. Removed. We were using them as a way to trace the call stack for debugging. https://codereview.chromium.org/1900423004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/CompositorProxyClientImpl.h (right): https://codereview.chromium.org/1900423004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/CompositorProxyClientImpl.h:49: typedef HeapHashMap<uint64_t, Member<ProxyVector>> ProxyMap; On 2016/06/08 00:10:36, haraken wrote: > > Can you use a HashMap/HashSet of WeakMembers? Then you don't need to call > unregisterCompositorProxy because the weak processing will automatically remove > the entries when the CompositorProxy objects get destructed. Done.
https://codereview.chromium.org/1900423004/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/compositor-proxy-postmessage.html (right): https://codereview.chromium.org/1900423004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/compositor-proxy-postmessage.html:7: var test = async_test("This test checks that an element's compositor proxy can be sent from the main thread to the compositor thread."); nit: please update the test description (e.g. "to the compositor thread and back again"). https://codereview.chromium.org/1900423004/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/resources/proxy-echo.js (right): https://codereview.chromium.org/1900423004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/resources/proxy-echo.js:3: if (typeof proxy == "CompositorProxy") By the way, what's this condition checking? How can 'typeof' ever return "CompositorProxy" (rather than "object")? https://codereview.chromium.org/1900423004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/CompositorProxy.cpp (right): https://codereview.chromium.org/1900423004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/CompositorProxy.cpp:142: Platform::current()->mainThread()->getWebTaskRunner()->postTask(BLINK_FROM_HERE, threadSafeBind(&incrementCompositorProxiedPropertiesForElement, m_elementId, m_compositorMutableProperties)); Do you really want to do this asynchronously when the message is received on the other thread? It would seem to me that you'd prefer to have the ref incremented on the main thread when the message is passed. My reasoning is this sequence of events: - main thread creates CompositorProxy - incrementCompositorProxiedPropertiesForElement, element needs style recalc - main thread posts message to compositor proxy - document lifecycle runs, beginning with style recalc (possibly inducing layout, compositing update, paint, etc.) - main thread object loses last reference and is collected - decrementCompositorProxiedPropertiesForElement, element needs style recalc - document lifecycle runs, beginning with style recalc (possibly inducing layout, compositing update, paint, etc.) - CompositorProxy deserializes message and builds a CompositorProxy object - incrementCompositorProxiedPropertiesForElement, element needs style recalc - document lifecycle runs, beginning with style recalc (possibly inducing layout, compositing update, paint, etc.) Or can this sequence not happen for some reason I'm missing?
Whoops, a couple typos in my previous message. On 2016/06/09 at 21:13:47, jbroman wrote: > https://codereview.chromium.org/1900423004/diff/100001/third_party/WebKit/Lay... > File third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/compositor-proxy-postmessage.html (right): > > https://codereview.chromium.org/1900423004/diff/100001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/compositor-proxy-postmessage.html:7: var test = async_test("This test checks that an element's compositor proxy can be sent from the main thread to the compositor thread."); > nit: please update the test description (e.g. "to the compositor thread and back again"). > > https://codereview.chromium.org/1900423004/diff/100001/third_party/WebKit/Lay... > File third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/resources/proxy-echo.js (right): > > https://codereview.chromium.org/1900423004/diff/100001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/resources/proxy-echo.js:3: if (typeof proxy == "CompositorProxy") > By the way, what's this condition checking? How can 'typeof' ever return "CompositorProxy" (rather than "object")? > > https://codereview.chromium.org/1900423004/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/dom/CompositorProxy.cpp (right): > > https://codereview.chromium.org/1900423004/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/CompositorProxy.cpp:142: Platform::current()->mainThread()->getWebTaskRunner()->postTask(BLINK_FROM_HERE, threadSafeBind(&incrementCompositorProxiedPropertiesForElement, m_elementId, m_compositorMutableProperties)); > Do you really want to do this asynchronously when the message is received on the other thread? It would seem to me that you'd prefer to have the ref incremented on the main thread when the message is passed. > > My reasoning is this sequence of events: > - main thread creates CompositorProxy > - incrementCompositorProxiedPropertiesForElement, element needs style recalc > - main thread posts message to compositor proxy posts message to compositor worker, rather > - document lifecycle runs, beginning with style recalc (possibly inducing layout, compositing update, paint, etc.) > - main thread object loses last reference and is collected > - decrementCompositorProxiedPropertiesForElement, element needs style recalc > - document lifecycle runs, beginning with style recalc (possibly inducing layout, compositing update, paint, etc.) > - CompositorProxy deserializes message and builds a CompositorProxy object compositor worker deserialized message > - incrementCompositorProxiedPropertiesForElement, element needs style recalc > - document lifecycle runs, beginning with style recalc (possibly inducing layout, compositing update, paint, etc.) > > Or can this sequence not happen for some reason I'm missing?
On 2016/06/09 21:13:47, jbroman wrote: > https://codereview.chromium.org/1900423004/diff/100001/third_party/WebKit/Lay... > File > third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/compositor-proxy-postmessage.html > (right): > > https://codereview.chromium.org/1900423004/diff/100001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/compositor-proxy-postmessage.html:7: > var test = async_test("This test checks that an element's compositor proxy can > be sent from the main thread to the compositor thread."); > nit: please update the test description (e.g. "to the compositor thread and back > again"). > > https://codereview.chromium.org/1900423004/diff/100001/third_party/WebKit/Lay... > File > third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/resources/proxy-echo.js > (right): > > https://codereview.chromium.org/1900423004/diff/100001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/resources/proxy-echo.js:3: > if (typeof proxy == "CompositorProxy") > By the way, what's this condition checking? How can 'typeof' ever return > "CompositorProxy" (rather than "object")? > > https://codereview.chromium.org/1900423004/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/dom/CompositorProxy.cpp (right): > > https://codereview.chromium.org/1900423004/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/CompositorProxy.cpp:142: > Platform::current()->mainThread()->getWebTaskRunner()->postTask(BLINK_FROM_HERE, > threadSafeBind(&incrementCompositorProxiedPropertiesForElement, m_elementId, > m_compositorMutableProperties)); > Do you really want to do this asynchronously when the message is received on the > other thread? It would seem to me that you'd prefer to have the ref incremented > on the main thread when the message is passed. > > My reasoning is this sequence of events: > - main thread creates CompositorProxy > - incrementCompositorProxiedPropertiesForElement, element needs style recalc > - main thread posts message to compositor proxy > - document lifecycle runs, beginning with style recalc (possibly inducing > layout, compositing update, paint, etc.) > - main thread object loses last reference and is collected > - decrementCompositorProxiedPropertiesForElement, element needs style recalc > - document lifecycle runs, beginning with style recalc (possibly inducing > layout, compositing update, paint, etc.) > - CompositorProxy deserializes message and builds a CompositorProxy object > - incrementCompositorProxiedPropertiesForElement, element needs style recalc > - document lifecycle runs, beginning with style recalc (possibly inducing > layout, compositing update, paint, etc.) > > Or can this sequence not happen for some reason I'm missing? Hmmm, it think what you are suggesting is possible. If I do worker.postMessage([new CompositorProxy($el)]); The main side proxy is valid during the post message but may be collected once the message is constructed and before it is received on the worker which can lead to composited prop count to go back and forth between 0-1 which causes promotion and de-promotion. If I understand correctly, your concern is that this may cause excessive style recalcs which can be expensive. >It would seem to me that you'd prefer to have the ref incremented > on the main thread when the message is passed. Perhaps. Though it may be a bit complicated because if we increment we need to guarantee that we decrement as well. which needs careful consideration given that this is tied to the message. I filed a bug to track this: https://bugs.chromium.org/p/chromium/issues/detail?id=619042 I think this should be addressed by a separate CL. This CL does not change the behaviour anyways.
https://codereview.chromium.org/1900423004/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/compositor-proxy-postmessage.html (right): https://codereview.chromium.org/1900423004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/compositor-proxy-postmessage.html:7: var test = async_test("This test checks that an element's compositor proxy can be sent from the main thread to the compositor thread."); On 2016/06/09 21:13:47, jbroman wrote: > nit: please update the test description (e.g. "to the compositor thread and back > again"). Done. https://codereview.chromium.org/1900423004/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/resources/proxy-echo.js (right): https://codereview.chromium.org/1900423004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/resources/proxy-echo.js:3: if (typeof proxy == "CompositorProxy") On 2016/06/09 21:13:47, jbroman wrote: > By the way, what's this condition checking? How can 'typeof' ever return > "CompositorProxy" (rather than "object")? Done. + some other minor cleanups.
https://codereview.chromium.org/1900423004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/CompositorProxy.h (right): https://codereview.chromium.org/1900423004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/CompositorProxy.h:56: void dispose(); This method seems neither implemented nor called; am I missing something? https://codereview.chromium.org/1900423004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/CompositorProxyClient.h (right): https://codereview.chromium.org/1900423004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/CompositorProxyClient.h:34: virtual void unregisterCompositorProxy(CompositorProxy*) = 0; Does this method have any call sites? I see the registration call site, but not the deregistration site. https://codereview.chromium.org/1900423004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp (right): https://codereview.chromium.org/1900423004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp:71: ProxyMap::AddResult entry = m_proxyMap.add(elementId, new ProxySet); This will create a spurious "new ProxySet" every time, which you don't want to do. Instead, only allocate it if there wasn't one previously. Similarly, the "contains" check is redundant: it's legal (and faster) to unconditionally add it (the set itself will not contain duplicate entries, and will only scan the hashtable once this way). ProxyMap::AddResult entry = m_proxyMap.add(proxy->elementId(), nullptr); Member<ProxySet>& proxies = entry.storedValue->value; if (entry.isNewEntry) proxies = new ProxySet; proxies->add(proxy); https://codereview.chromium.org/1900423004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp:83: m_proxyMap.remove(elementId); This case is probably less worrisome, but this is still one more hashtable scan than necessary. You might consider this pattern instead (slightly more complicated, but only one line longer): ProxyMap::iterator it = m_proxyMap.find(proxy->elementId()); if (it == m_proxyMap.end()) return; ProxySet* proxies = it->value; proxies->remove(proxy); if (proxies->isEmpty()) m_proxyMap.remove(it); ...but I'm less worried about this than the extra allocation above. https://codereview.chromium.org/1900423004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/CompositorProxyClientImpl.h (right): https://codereview.chromium.org/1900423004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/CompositorProxyClientImpl.h:23: class CompositorProxyClientImpl final : public GarbageCollectedFinalized<CompositorProxyClientImpl>, public CompositorProxyClient { Why did this class become GarbageCollectedFinalized? I only see the addition of a GC-ed member, which shouldn't require finalization.
On 2016/06/10 at 15:11:23, majidvp wrote: > On 2016/06/09 21:13:47, jbroman wrote: > > https://codereview.chromium.org/1900423004/diff/100001/third_party/WebKit/Lay... > > File > > third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/compositor-proxy-postmessage.html > > (right): > > > > https://codereview.chromium.org/1900423004/diff/100001/third_party/WebKit/Lay... > > third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/compositor-proxy-postmessage.html:7: > > var test = async_test("This test checks that an element's compositor proxy can > > be sent from the main thread to the compositor thread."); > > nit: please update the test description (e.g. "to the compositor thread and back > > again"). > > > > https://codereview.chromium.org/1900423004/diff/100001/third_party/WebKit/Lay... > > File > > third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/resources/proxy-echo.js > > (right): > > > > https://codereview.chromium.org/1900423004/diff/100001/third_party/WebKit/Lay... > > third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/resources/proxy-echo.js:3: > > if (typeof proxy == "CompositorProxy") > > By the way, what's this condition checking? How can 'typeof' ever return > > "CompositorProxy" (rather than "object")? > > > > https://codereview.chromium.org/1900423004/diff/100001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/dom/CompositorProxy.cpp (right): > > > > https://codereview.chromium.org/1900423004/diff/100001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/dom/CompositorProxy.cpp:142: > > Platform::current()->mainThread()->getWebTaskRunner()->postTask(BLINK_FROM_HERE, > > threadSafeBind(&incrementCompositorProxiedPropertiesForElement, m_elementId, > > m_compositorMutableProperties)); > > Do you really want to do this asynchronously when the message is received on the > > other thread? It would seem to me that you'd prefer to have the ref incremented > > on the main thread when the message is passed. > > > > My reasoning is this sequence of events: > > - main thread creates CompositorProxy > > - incrementCompositorProxiedPropertiesForElement, element needs style recalc > > - main thread posts message to compositor proxy > > - document lifecycle runs, beginning with style recalc (possibly inducing > > layout, compositing update, paint, etc.) > > - main thread object loses last reference and is collected > > - decrementCompositorProxiedPropertiesForElement, element needs style recalc > > - document lifecycle runs, beginning with style recalc (possibly inducing > > layout, compositing update, paint, etc.) > > - CompositorProxy deserializes message and builds a CompositorProxy object > > - incrementCompositorProxiedPropertiesForElement, element needs style recalc > > - document lifecycle runs, beginning with style recalc (possibly inducing > > layout, compositing update, paint, etc.) > > > > Or can this sequence not happen for some reason I'm missing? > > Hmmm, it think what you are suggesting is possible. > If I do > worker.postMessage([new CompositorProxy($el)]); > > The main side proxy is valid during the post message but > may be collected once the message is constructed and > before it is received on the worker which can lead to composited prop > count to go back and forth between 0-1 which causes promotion > and de-promotion. > > If I understand correctly, your concern is that this may cause > excessive style recalcs which can be expensive. I think it might also cause user-visible behaviour, because the style recalc could cause layout (since, e.g., whether transform is proxied affects whether elements establish containing blocks), and thus the appearance of the page. It's possible that the fact that the thread you're expecting to send to is the compositor thread might save us from drawing some of those bad frames, but this could be visible in other ways (since JS can observe layout), or when passing a compositor proxy to a worker with no such synchronization (AFAICT the author _can_ do that, even if it seems unusual to do so). > >It would seem to me that you'd prefer to have the ref incremented > > on the main thread when the message is passed. > > Perhaps. Though it may be a bit complicated because if > we increment we need to guarantee that we decrement as well. > which needs careful consideration given that this is tied to the > message. > > I filed a bug to track this: > https://bugs.chromium.org/p/chromium/issues/detail?id=619042 > > I think this should be addressed by a separate CL. This CL does not > change the behaviour anyways. Fair enough, I won't block this CL on it. Thanks for filing.
https://codereview.chromium.org/1900423004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/CompositorProxy.h (right): https://codereview.chromium.org/1900423004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/CompositorProxy.h:56: void dispose(); On 2016/06/10 17:45:42, jbroman wrote: > This method seems neither implemented nor called; am I missing something? Ah! Removed. https://codereview.chromium.org/1900423004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/CompositorProxyClient.h (right): https://codereview.chromium.org/1900423004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/CompositorProxyClient.h:34: virtual void unregisterCompositorProxy(CompositorProxy*) = 0; On 2016/06/10 17:45:42, jbroman wrote: > Does this method have any call sites? I see the registration call site, but not > the deregistration site. It should be used in |CompositorProxy::disconnet| which may be called by script. I was to hasty to remove that call site when switched to using weak map. It is corrected now. https://codereview.chromium.org/1900423004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp (right): https://codereview.chromium.org/1900423004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp:71: ProxyMap::AddResult entry = m_proxyMap.add(elementId, new ProxySet); On 2016/06/10 17:45:42, jbroman wrote: > This will create a spurious "new ProxySet" every time, which you don't want to > do. Instead, only allocate it if there wasn't one previously. Similarly, the > "contains" check is redundant: it's legal (and faster) to unconditionally add it > (the set itself will not contain duplicate entries, and will only scan the > hashtable once this way). > > ProxyMap::AddResult entry = m_proxyMap.add(proxy->elementId(), nullptr); > Member<ProxySet>& proxies = entry.storedValue->value; > if (entry.isNewEntry) > proxies = new ProxySet; > proxies->add(proxy); Great! I like this pattern of using isNewEntry. Thanks. https://codereview.chromium.org/1900423004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp:83: m_proxyMap.remove(elementId); On 2016/06/10 17:45:42, jbroman wrote: > This case is probably less worrisome, but this is still one more hashtable scan > than necessary. You might consider this pattern instead (slightly more > complicated, but only one line longer): > > ProxyMap::iterator it = m_proxyMap.find(proxy->elementId()); > if (it == m_proxyMap.end()) > return; > > ProxySet* proxies = it->value; > proxies->remove(proxy); > if (proxies->isEmpty()) > m_proxyMap.remove(it); > > ...but I'm less worried about this than the extra allocation above. Done. https://codereview.chromium.org/1900423004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/CompositorProxyClientImpl.h (right): https://codereview.chromium.org/1900423004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/CompositorProxyClientImpl.h:23: class CompositorProxyClientImpl final : public GarbageCollectedFinalized<CompositorProxyClientImpl>, public CompositorProxyClient { On 2016/06/10 17:45:42, jbroman wrote: > Why did this class become GarbageCollectedFinalized? I only see the addition of > a GC-ed member, which shouldn't require finalization. Hmmm, I recall getting warning from oilpan that it was needed but I don't anymore. So removed.
lgtm, but please make sure to get approval from an Oilpan expert, as there are tricky lifetime things going on here https://codereview.chromium.org/1900423004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/CompositorProxy.cpp (right): https://codereview.chromium.org/1900423004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/CompositorProxy.cpp:164: DCHECK(!m_connected); Mind a quick comment here explaining why you don't call unregisterCompositorProxy here? (I assume it's because the client is holds a weak set.) https://codereview.chromium.org/1900423004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/CompositorProxyClient.h (right): https://codereview.chromium.org/1900423004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/CompositorProxyClient.h:33: virtual void registerCompositorProxy(CompositorProxy*) = 0; Please add comments clarifying the semantics of registration/unregistration. In particular, there is the subtle point that "unregister" will only be called for explicit disconnect calls, but that subclasses are expected to also handle the object being collected (e.g. using WeakMember) without being unregistered first. https://codereview.chromium.org/1900423004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/CompositorProxyClientImpl.h (right): https://codereview.chromium.org/1900423004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/web/CompositorProxyClientImpl.h:49: typedef HeapHashMap<uint64_t, Member<ProxySet>> ProxyMap; One thing to watch out for here: if the last object to die here dies without being explicitly disconnected, the set will become empty but the entry won't be removed from the map. That may cause memory to increase over time. (But I don't know what the ideal way to handle this from an Oilpan standpoint is; haraken can probably offer advice.)
> https://codereview.chromium.org/1900423004/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/web/CompositorProxyClientImpl.h:49: typedef > HeapHashMap<uint64_t, Member<ProxySet>> ProxyMap; > One thing to watch out for here: if the last object to die here dies without > being explicitly disconnected, the set will become empty but the entry won't be > removed from the map. That may cause memory to increase over time. > > (But I don't know what the ideal way to handle this from an Oilpan standpoint > is; haraken can probably offer advice.) haraken@: PTAL. Specially I appreciate any advice you may have regarding the clean up issue that we have when using map of weak sets. HeapHashMap<id, HeapHashSet<Weak<proxy>> Essentially I use the above structure and want to make sure we remove the map entry once the weak set is emptied. This was trivial with pre-finalizer but I am not sure what is the right approach here:
https://codereview.chromium.org/1900423004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/CompositorProxy.cpp (right): https://codereview.chromium.org/1900423004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/CompositorProxy.cpp:164: DCHECK(!m_connected); On 2016/06/10 19:29:33, jbroman wrote: > Mind a quick comment here explaining why you don't call > unregisterCompositorProxy here? (I assume it's because the client is holds a > weak set.) Done. https://codereview.chromium.org/1900423004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/CompositorProxyClient.h (right): https://codereview.chromium.org/1900423004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/CompositorProxyClient.h:33: virtual void registerCompositorProxy(CompositorProxy*) = 0; On 2016/06/10 19:29:33, jbroman wrote: > Please add comments clarifying the semantics of registration/unregistration. In > particular, there is the subtle point that "unregister" will only be called for > explicit disconnect calls, but that subclasses are expected to also handle the > object being collected (e.g. using WeakMember) without being unregistered first. Done.
Oilpan part LGTM. The weak hash map will work as expected.
haraken@chromium.org changed reviewers: + sigbjornf@opera.com
> Would you mind pointing me at where this behaviour is documented? I'm aware that sets with weak contents and maps with weak keys > have special behaviour, but the expanded type here is: > > HeapHashMap<uint64_t, Member<HeapHashSet<WeakMember<CompositorProxy>>> > > While the inner set is weak, what makes the outer map weak? I'd have expected it to be strong due to the use of Member<>, keeping > the (empty) set alive even when its last element was removed unless it were explicitly removed from the map. I haven't been able > to understand why this works correctly (i.e. without leaking sets until the map is collected). Sorry, I was confused. I think that the outer map must be weak (sigbjorn should confirm this). HeapHashMap<uint64_t, WeakMember<HeapHashSet<WeakMember<CompositorProxy>>> Then the inner HeapHashSet will be automatically removed by the weak processing of Oilpan. The weak processing for collections that contain weak members is realized by a mechanism called ephemeron (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/HashTable....).
On 2016/06/13 14:50:30, haraken wrote: > > Would you mind pointing me at where this behaviour is documented? I'm aware > that sets with weak contents and maps with weak keys > have special behaviour, > but the expanded type here is: > > > > HeapHashMap<uint64_t, Member<HeapHashSet<WeakMember<CompositorProxy>>> > > > > While the inner set is weak, what makes the outer map weak? I'd have expected > it to be strong due to the use of Member<>, keeping > the (empty) set alive even > when its last element was removed unless it were explicitly removed from the > map. I haven't been able > to understand why this works correctly (i.e. without > leaking sets until the map is collected). > > Sorry, I was confused. I think that the outer map must be weak (sigbjorn should > confirm this). > > HeapHashMap<uint64_t, WeakMember<HeapHashSet<WeakMember<CompositorProxy>>> > > Then the inner HeapHashSet will be automatically removed by the weak processing > of Oilpan. The weak processing for collections that contain weak members is > realized by a mechanism called ephemeron > (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/HashTable....). Yes, that ought to behave as wanted. We could improve our unit test coverage for this case, but HeapTest.EphemeronsInEphemerons comes close.
The example in the HeapTest.EphemeronsInEphemerons uses weak keys, what we have here are weak values. does that make any difference? So If I read this correctly, you are suggesting that if I used a weak map of weak set [1], then and empty inner set will be automatically collected. That is exactly what I need. Note that we don't have any cycle between key, values here. [1] HeapHashMap<uint64_t, WeakMember<HeapHashSet< WeakMember<CompositorProxy>>> On Mon, Jun 13, 2016 at 10:50 AM <haraken@chromium.org> wrote: > > Would you mind pointing me at where this behaviour is documented? I'm > aware > that sets with weak contents and maps with weak keys > have special > behaviour, > but the expanded type here is: > > > > HeapHashMap<uint64_t, Member<HeapHashSet<WeakMember<CompositorProxy>>> > > > > While the inner set is weak, what makes the outer map weak? I'd have > expected > it to be strong due to the use of Member<>, keeping > the (empty) set > alive even > when its last element was removed unless it were explicitly removed from > the > map. I haven't been able > to understand why this works correctly (i.e. > without > leaking sets until the map is collected). > > Sorry, I was confused. I think that the outer map must be weak (sigbjorn > should > confirm this). > > HeapHashMap<uint64_t, WeakMember<HeapHashSet<WeakMember<CompositorProxy>>> > > Then the inner HeapHashSet will be automatically removed by the weak > processing > of Oilpan. The weak processing for collections that contain weak members is > realized by a mechanism called ephemeron > ( > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/HashTable.... > ). > > > > https://codereview.chromium.org/1900423004/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2016/06/13 15:08:05, sof wrote: > On 2016/06/13 14:50:30, haraken wrote: > > > Would you mind pointing me at where this behaviour is documented? I'm aware > > that sets with weak contents and maps with weak keys > have special behaviour, > > but the expanded type here is: > > > > > > HeapHashMap<uint64_t, Member<HeapHashSet<WeakMember<CompositorProxy>>> > > > > > > While the inner set is weak, what makes the outer map weak? I'd have > expected > > it to be strong due to the use of Member<>, keeping > the (empty) set alive > even > > when its last element was removed unless it were explicitly removed from the > > map. I haven't been able > to understand why this works correctly (i.e. > without > > leaking sets until the map is collected). > > > > Sorry, I was confused. I think that the outer map must be weak (sigbjorn > should > > confirm this). > > > > HeapHashMap<uint64_t, WeakMember<HeapHashSet<WeakMember<CompositorProxy>>> > > > > Then the inner HeapHashSet will be automatically removed by the weak > processing > > of Oilpan. The weak processing for collections that contain weak members is > > realized by a mechanism called ephemeron > > > (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/HashTable....). > > Yes, that ought to behave as wanted. We could improve our unit test coverage for > this case, but HeapTest.EphemeronsInEphemerons comes close. No, looking at the behavior wanted, unless you have some separate strong reference (Member<>) to the inner hash set, this won't work. Only thing/hack I can think of is to register a weak callback (via registerWeakMembers()), and check for emptiness of the sets & remove their entries. And register before tracing the hash map.
The example in the HeapTest.EphemeronsInEphemerons uses weak keys, what we have here are weak values. does that make any difference? So If I read this correctly, you are suggesting that if I used a weak map of weak set [1], then and empty inner set will be automatically collected. That is exactly what I need. Note that we don't have any cycle between key, values here. [1] HeapHashMap<uint64_t, WeakMember<HeapHashSet< WeakMember<CompositorProxy>>> On Mon, Jun 13, 2016 at 10:50 AM <haraken@chromium.org> wrote: > > Would you mind pointing me at where this behaviour is documented? I'm > aware > that sets with weak contents and maps with weak keys > have special > behaviour, > but the expanded type here is: > > > > HeapHashMap<uint64_t, Member<HeapHashSet<WeakMember<CompositorProxy>>> > > > > While the inner set is weak, what makes the outer map weak? I'd have > expected > it to be strong due to the use of Member<>, keeping > the (empty) set > alive even > when its last element was removed unless it were explicitly removed from > the > map. I haven't been able > to understand why this works correctly (i.e. > without > leaking sets until the map is collected). > > Sorry, I was confused. I think that the outer map must be weak (sigbjorn > should > confirm this). > > HeapHashMap<uint64_t, WeakMember<HeapHashSet<WeakMember<CompositorProxy>>> > > Then the inner HeapHashSet will be automatically removed by the weak > processing > of Oilpan. The weak processing for collections that contain weak members is > realized by a mechanism called ephemeron > ( > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/HashTable.... > ). > > > > https://codereview.chromium.org/1900423004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/06/13 15:25:30, sof wrote: > On 2016/06/13 15:08:05, sof wrote: > > On 2016/06/13 14:50:30, haraken wrote: > > > > Would you mind pointing me at where this behaviour is documented? I'm > aware > > > that sets with weak contents and maps with weak keys > have special > behaviour, > > > but the expanded type here is: > > > > > > > > HeapHashMap<uint64_t, Member<HeapHashSet<WeakMember<CompositorProxy>>> > > > > > > > > While the inner set is weak, what makes the outer map weak? I'd have > > expected > > > it to be strong due to the use of Member<>, keeping > the (empty) set alive > > even > > > when its last element was removed unless it were explicitly removed from the > > > map. I haven't been able > to understand why this works correctly (i.e. > > without > > > leaking sets until the map is collected). > > > > > > Sorry, I was confused. I think that the outer map must be weak (sigbjorn > > should > > > confirm this). > > > > > > HeapHashMap<uint64_t, WeakMember<HeapHashSet<WeakMember<CompositorProxy>>> > > > > > > Then the inner HeapHashSet will be automatically removed by the weak > > processing > > > of Oilpan. The weak processing for collections that contain weak members is > > > realized by a mechanism called ephemeron > > > > > > (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/HashTable....). > > > > Yes, that ought to behave as wanted. We could improve our unit test coverage > for > > this case, but HeapTest.EphemeronsInEphemerons comes close. > > No, looking at the behavior wanted, unless you have some separate strong > reference (Member<>) to the inner hash set, this won't work. > > Only thing/hack I can think of is to register a weak callback (via > registerWeakMembers()), and check for emptiness of the sets & remove their > entries. And register before tracing the hash map. An alternative is to manually GC the hash map of empty entries upon the next addition, I suppose. Making the GC work overtime for what is a small table(?), would be the trade-off.
It turned out we don't actually need the map[id->weak set] and I can replace it with a weak set. This simplification avoids this issue altogether. Thanks! On Mon, Jun 13, 2016 at 11:30 AM <sigbjornf@opera.com> wrote: > On 2016/06/13 15:25:30, sof wrote: > > On 2016/06/13 15:08:05, sof wrote: > > > On 2016/06/13 14:50:30, haraken wrote: > > > > > Would you mind pointing me at where this behaviour is documented? > I'm > > aware > > > > that sets with weak contents and maps with weak keys > have special > > behaviour, > > > > but the expanded type here is: > > > > > > > > > > HeapHashMap<uint64_t, > Member<HeapHashSet<WeakMember<CompositorProxy>>> > > > > > > > > > > While the inner set is weak, what makes the outer map weak? I'd > have > > > expected > > > > it to be strong due to the use of Member<>, keeping > the (empty) set > alive > > > even > > > > when its last element was removed unless it were explicitly removed > from > the > > > > map. I haven't been able > to understand why this works correctly > (i.e. > > > without > > > > leaking sets until the map is collected). > > > > > > > > Sorry, I was confused. I think that the outer map must be weak > (sigbjorn > > > should > > > > confirm this). > > > > > > > > HeapHashMap<uint64_t, > WeakMember<HeapHashSet<WeakMember<CompositorProxy>>> > > > > > > > > Then the inner HeapHashSet will be automatically removed by the weak > > > processing > > > > of Oilpan. The weak processing for collections that contain weak > members > is > > > > realized by a mechanism called ephemeron > > > > > > > > > > ( > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/HashTable.... > ). > > > > > > Yes, that ought to behave as wanted. We could improve our unit test > coverage > > for > > > this case, but HeapTest.EphemeronsInEphemerons comes close. > > > > No, looking at the behavior wanted, unless you have some separate strong > > reference (Member<>) to the inner hash set, this won't work. > > > > Only thing/hack I can think of is to register a weak callback (via > > registerWeakMembers()), and check for emptiness of the sets & remove > their > > entries. And register before tracing the hash map. > > An alternative is to manually GC the hash map of empty entries upon the > next > addition, I suppose. Making the GC work overtime for what is a small > table(?), > would be the trade-off. > > https://codereview.chromium.org/1900423004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
It turned out we don't actually need the map[id->weak set] and I can replace it with a weak set. This simplification avoids this issue altogether. Thanks! On Mon, Jun 13, 2016 at 11:30 AM <sigbjornf@opera.com> wrote: > On 2016/06/13 15:25:30, sof wrote: > > On 2016/06/13 15:08:05, sof wrote: > > > On 2016/06/13 14:50:30, haraken wrote: > > > > > Would you mind pointing me at where this behaviour is documented? > I'm > > aware > > > > that sets with weak contents and maps with weak keys > have special > > behaviour, > > > > but the expanded type here is: > > > > > > > > > > HeapHashMap<uint64_t, > Member<HeapHashSet<WeakMember<CompositorProxy>>> > > > > > > > > > > While the inner set is weak, what makes the outer map weak? I'd > have > > > expected > > > > it to be strong due to the use of Member<>, keeping > the (empty) set > alive > > > even > > > > when its last element was removed unless it were explicitly removed > from > the > > > > map. I haven't been able > to understand why this works correctly > (i.e. > > > without > > > > leaking sets until the map is collected). > > > > > > > > Sorry, I was confused. I think that the outer map must be weak > (sigbjorn > > > should > > > > confirm this). > > > > > > > > HeapHashMap<uint64_t, > WeakMember<HeapHashSet<WeakMember<CompositorProxy>>> > > > > > > > > Then the inner HeapHashSet will be automatically removed by the weak > > > processing > > > > of Oilpan. The weak processing for collections that contain weak > members > is > > > > realized by a mechanism called ephemeron > > > > > > > > > > ( > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/HashTable.... > ). > > > > > > Yes, that ought to behave as wanted. We could improve our unit test > coverage > > for > > > this case, but HeapTest.EphemeronsInEphemerons comes close. > > > > No, looking at the behavior wanted, unless you have some separate strong > > reference (Member<>) to the inner hash set, this won't work. > > > > Only thing/hack I can think of is to register a weak callback (via > > registerWeakMembers()), and check for emptiness of the sets & remove > their > > entries. And register before tracing the hash map. > > An alternative is to manually GC the hash map of empty entries upon the > next > addition, I suppose. Making the GC work overtime for what is a small > table(?), > would be the trade-off. > > https://codereview.chromium.org/1900423004/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Great! (I'd assumed a subsequent patch required multi-map behaviour, but if not this is a nice simplification.) Still LGTM. https://codereview.chromium.org/1900423004/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/CompositorProxyClient.h (right): https://codereview.chromium.org/1900423004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/CompositorProxyClient.h:37: // proxy from our weak map. There is no longer a weak map. Either be vague ("to remove the weak reference to the proxy" or similar) or update it to say "set".
https://codereview.chromium.org/1900423004/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/CompositorProxyClient.h (right): https://codereview.chromium.org/1900423004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/CompositorProxyClient.h:37: // proxy from our weak map. On 2016/06/13 17:15:05, jbroman wrote: > There is no longer a weak map. Either be vague ("to remove the weak reference to > the proxy" or similar) or update it to say "set". Done.
The CQ bit was checked by majidvp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, jbroman@chromium.org Link to the patchset: https://codereview.chromium.org/1900423004/#ps220001 (title: "Update comment")
The CQ bit was unchecked by majidvp@chromium.org
Description was changed from ========== [compositorworker] Register compositor proxies with proxy client Each compositor proxy registers itself with its proxy client when created. This populated map is later used to update the backing mutable state for each compositor proxy. TEST=These machinery is tested in the follow-up CL https://codereview.chromium.org/2041193005/ BUG=430155 ========== to ========== [compositorworker] Register compositor proxies with proxy client Each compositor proxy registers itself with its proxy client when created. This populated set is later used to update the backing mutable state for each compositor proxy. TEST=These machinery is tested in the follow-up CL https://codereview.chromium.org/2041193005/ BUG=430155 ==========
The CQ bit was checked by majidvp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1900423004/220001
Message was sent while issue was closed.
Description was changed from ========== [compositorworker] Register compositor proxies with proxy client Each compositor proxy registers itself with its proxy client when created. This populated set is later used to update the backing mutable state for each compositor proxy. TEST=These machinery is tested in the follow-up CL https://codereview.chromium.org/2041193005/ BUG=430155 ========== to ========== [compositorworker] Register compositor proxies with proxy client Each compositor proxy registers itself with its proxy client when created. This populated set is later used to update the backing mutable state for each compositor proxy. TEST=These machinery is tested in the follow-up CL https://codereview.chromium.org/2041193005/ BUG=430155 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== [compositorworker] Register compositor proxies with proxy client Each compositor proxy registers itself with its proxy client when created. This populated set is later used to update the backing mutable state for each compositor proxy. TEST=These machinery is tested in the follow-up CL https://codereview.chromium.org/2041193005/ BUG=430155 ========== to ========== [compositorworker] Register compositor proxies with proxy client Each compositor proxy registers itself with its proxy client when created. This populated set is later used to update the backing mutable state for each compositor proxy. TEST=These machinery is tested in the follow-up CL https://codereview.chromium.org/2041193005/ BUG=430155 Committed: https://crrev.com/eb14272233cd88227efc37a31fabb9688ed831ba Cr-Commit-Position: refs/heads/master@{#399502} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/eb14272233cd88227efc37a31fabb9688ed831ba Cr-Commit-Position: refs/heads/master@{#399502} |
