Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2268153002:
Make PendingScript MemoryCoordinatorClient. (Closed)
|
Created:
4 years, 3 months ago by tasak Modified:
4 years, 3 months ago CC:
chromium-reviews, sof, eae+blinkwatch, blink-reviews-dom_chromium.org, dglazkov+blink, blink-reviews, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake PendingScript MemoryCoordinatorClient:
- implement prepareToSuspend for background renderer's purge + suspend. prepareToSuspend is invoked before a background renderer is suspended.
- PendingScript's prepareToSuspend cancels ScriptStreaming.
- intent-to-implement-and-ship of background renderer's purge + suspend is https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4
- one of the documents attached in the above intent is https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJvI/edit?usp=sharing
BUG=635419
Committed: https://crrev.com/2635c95b75dd28be2678f1e4d61f72ff46192d27
Cr-Commit-Position: refs/heads/master@{#414404}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Removed unregisterClient. #Patch Set 3 : Check only whether m_streamer exists or not. #
Messages
Total messages: 38 (19 generated)
The CQ bit was checked by tasak@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Make PendingScript MemoryCoordinatorClient. BUG= ========== to ========== Make PendingScript MemoryCoordinatorClient: - implement prepareToSuspend to cancel ScriptStreaming before suspending a renderer. BUG=635419 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tasak@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Make PendingScript MemoryCoordinatorClient: - implement prepareToSuspend to cancel ScriptStreaming before suspending a renderer. BUG=635419 ========== to ========== Make PendingScript MemoryCoordinatorClient: - implement prepareToSuspend to cancel ScriptStreaming before suspending a background renderer. - background renderer suspend is explained in https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ... BUG=635419 ==========
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by tasak@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
tasak@google.com changed reviewers: + haraken@chromium.org, marja@chromium.org
marja, would you review this CL? I revised the previous patch: https://codereview.chromium.org/2251893002/ I discussed with hiroshige@: - It is a little dangerous to update Resource::prune() logic. It might cause some regression. - ScriptStreaming is working while Resource is loading, but prune() doesn't do anything for loading resources. But touching loading resources in MemoryCache is not good idea. So I made PendingScript MemoryCoordinatorClient. So prepareToSuspend is invoked from MemoryCoordinator when MemoryCoordinator decides to suspend a background renderer.
https://codereview.chromium.org/2268153002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/PendingScript.cpp (right): https://codereview.chromium.org/2268153002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/PendingScript.cpp:56: MemoryCoordinator::instance().unregisterClient(this); You don't need to (shouldn't) call this. Oilpan's weak processing automatically removes the client from the hash set. https://codereview.chromium.org/2268153002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/PendingScript.cpp:235: if (resource() && resource()->isLoaded()) Would you help me understand why we need this check?
https://codereview.chromium.org/2268153002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/PendingScript.cpp (right): https://codereview.chromium.org/2268153002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/PendingScript.cpp:56: MemoryCoordinator::instance().unregisterClient(this); On 2016/08/24 06:11:12, haraken wrote: > > You don't need to (shouldn't) call this. Oilpan's weak processing automatically > removes the client from the hash set. Done. https://codereview.chromium.org/2268153002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/PendingScript.cpp:235: if (resource() && resource()->isLoaded()) On 2016/08/24 06:11:12, haraken wrote: > > Would you help me understand why we need this check? When I chatted with hiroshige@, he said that ScriptStreaming had already finished when a resource was loaded. So we don't need to do anything when a script resource is loaded. MemoryCache should take care of loaded resources (or resource's prepareToSuspend should take care).
On 2016/08/24 07:44:54, tasak wrote: > https://codereview.chromium.org/2268153002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/dom/PendingScript.cpp (right): > > https://codereview.chromium.org/2268153002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/dom/PendingScript.cpp:56: > MemoryCoordinator::instance().unregisterClient(this); > On 2016/08/24 06:11:12, haraken wrote: > > > > You don't need to (shouldn't) call this. Oilpan's weak processing > automatically > > removes the client from the hash set. > > Done. > > https://codereview.chromium.org/2268153002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/dom/PendingScript.cpp:235: if (resource() && > resource()->isLoaded()) > On 2016/08/24 06:11:12, haraken wrote: > > > > Would you help me understand why we need this check? > > When I chatted with hiroshige@, he said that ScriptStreaming had already > finished when a resource was loaded. So we don't need to do anything when a > script resource is loaded. MemoryCache should take care of loaded resources (or > resource's prepareToSuspend should take care). I was assuming that m_streamer is cleared when ScriptStreaming has finished. Isn't it cleared?
https://codereview.chromium.org/2268153002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/PendingScript.cpp (right): https://codereview.chromium.org/2268153002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/PendingScript.cpp:235: if (resource() && resource()->isLoaded()) On 2016/08/24 07:44:53, tasak wrote: > On 2016/08/24 06:11:12, haraken wrote: > > > > Would you help me understand why we need this check? > > When I chatted with hiroshige@, he said that ScriptStreaming had already > finished when a resource was loaded. So we don't need to do anything when a > script resource is loaded. MemoryCache should take care of loaded resources (or > resource's prepareToSuspend should take care). > > If the resource is loaded we might still be parsing it. Don't you want to cancel the parsing? Also, doesn't finishing the parsing then make the upper layers compile and run the script, or, which part takes care of cancelling / suppressing those actions?
Thank you for review. https://codereview.chromium.org/2268153002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/PendingScript.cpp (right): https://codereview.chromium.org/2268153002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/PendingScript.cpp:235: if (resource() && resource()->isLoaded()) On 2016/08/24 09:02:55, marja wrote: > On 2016/08/24 07:44:53, tasak wrote: > > On 2016/08/24 06:11:12, haraken wrote: > > > > > > Would you help me understand why we need this check? > > > > When I chatted with hiroshige@, he said that ScriptStreaming had already > > finished when a resource was loaded. So we don't need to do anything when a > > script resource is loaded. MemoryCache should take care of loaded resources > (or > > resource's prepareToSuspend should take care). > > > > > > If the resource is loaded we might still be parsing it. Don't you want to cancel > the parsing? > > Also, doesn't finishing the parsing then make the upper layers compile and run > the script, or, which part takes care of cancelling / suppressing those actions? In the case, I would like to update ScriptResource (or its base class Resource) to do some action (if possible, cancel parsing). i.e. ScriptResource::prepareToSuspend() { /* cancel parsing? */ }
I might not see the bigger picture of how this will work... But isn't code in ScriptResource to cancel parsing in a wrong abstraction layer - ScriptResource shouldn't know anything about whether it's streamed or not; ScriptStreamer should know it (and PendingScript should only know the minimal it has to, by pointing to the ScriptStreamer).
On 2016/08/24 11:05:12, marja wrote: > I might not see the bigger picture of how this will work... > > But isn't code in ScriptResource to cancel parsing in a wrong abstraction layer > - ScriptResource shouldn't know anything about whether it's streamed or not; > ScriptStreamer should know it (and PendingScript should only know the minimal it > has to, by pointing to the ScriptStreamer). I see. I missed the ScriptStreamer's behavior.... ScriptStreamer::isFinished() is checking if both loading and parsing are finished... So is it ok to always invoke m_streamer->cancel() if m_streamer != null?
Description was changed from ========== Make PendingScript MemoryCoordinatorClient: - implement prepareToSuspend to cancel ScriptStreaming before suspending a background renderer. - background renderer suspend is explained in https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ... BUG=635419 ========== to ========== Make PendingScript MemoryCoordinatorClient: - implement prepareToSuspend to cancel ScriptStreaming before suspending a background renderer. - intent-to-implement-and-ship is https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4 - background renderer suspend is explained in https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ... BUG=635419 ==========
Description was changed from ========== Make PendingScript MemoryCoordinatorClient: - implement prepareToSuspend to cancel ScriptStreaming before suspending a background renderer. - intent-to-implement-and-ship is https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4 - background renderer suspend is explained in https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ... BUG=635419 ========== to ========== Make PendingScript MemoryCoordinatorClient: - implement prepareToSuspend to cancel ScriptStreaming before suspending a background renderer. - intent-to-implement-and-ship of background renderer's purge + suspend is https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4 - one of the documents attached in the above intent is https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ... BUG=635419 ==========
It should be.
Like, that's how it's intended to be used :) If it's not ok, you might need to fix it on the go. But it should work.
I see. Thank you, marja. On 2016/08/24 11:30:39, marja wrote: > Like, that's how it's intended to be used :) If it's not ok, you might need to > fix it on the go. But it should work. I uploaded Patch Set 3. It looks ok?
Description was changed from ========== Make PendingScript MemoryCoordinatorClient: - implement prepareToSuspend to cancel ScriptStreaming before suspending a background renderer. - intent-to-implement-and-ship of background renderer's purge + suspend is https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4 - one of the documents attached in the above intent is https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ... BUG=635419 ========== to ========== Make PendingScript MemoryCoordinatorClient: - implement prepareToSuspend for background renderer's purge + suspend. prepareToSuspend is invoked before a background renderer is suspended. - PendingScript's prepareToSuspend cancels ScriptStreaming. - intent-to-implement-and-ship of background renderer's purge + suspend is https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4 - one of the documents attached in the above intent is https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ... BUG=635419 ==========
Code lgtm. What kind of test did you use for testing that this actually does what you want it to do?
LGTM
Thank you.
The CQ bit was checked by tasak@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Make PendingScript MemoryCoordinatorClient: - implement prepareToSuspend for background renderer's purge + suspend. prepareToSuspend is invoked before a background renderer is suspended. - PendingScript's prepareToSuspend cancels ScriptStreaming. - intent-to-implement-and-ship of background renderer's purge + suspend is https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4 - one of the documents attached in the above intent is https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ... BUG=635419 ========== to ========== Make PendingScript MemoryCoordinatorClient: - implement prepareToSuspend for background renderer's purge + suspend. prepareToSuspend is invoked before a background renderer is suspended. - PendingScript's prepareToSuspend cancels ScriptStreaming. - intent-to-implement-and-ship of background renderer's purge + suspend is https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4 - one of the documents attached in the above intent is https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ... BUG=635419 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Make PendingScript MemoryCoordinatorClient: - implement prepareToSuspend for background renderer's purge + suspend. prepareToSuspend is invoked before a background renderer is suspended. - PendingScript's prepareToSuspend cancels ScriptStreaming. - intent-to-implement-and-ship of background renderer's purge + suspend is https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4 - one of the documents attached in the above intent is https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ... BUG=635419 ========== to ========== Make PendingScript MemoryCoordinatorClient: - implement prepareToSuspend for background renderer's purge + suspend. prepareToSuspend is invoked before a background renderer is suspended. - PendingScript's prepareToSuspend cancels ScriptStreaming. - intent-to-implement-and-ship of background renderer's purge + suspend is https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4 - one of the documents attached in the above intent is https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ... BUG=635419 Committed: https://crrev.com/2635c95b75dd28be2678f1e4d61f72ff46192d27 Cr-Commit-Position: refs/heads/master@{#414404} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2635c95b75dd28be2678f1e4d61f72ff46192d27 Cr-Commit-Position: refs/heads/master@{#414404}
Message was sent while issue was closed.
Hi again, did you see my question about tests above? :)
Message was sent while issue was closed.
On 2016/08/26 07:22:52, marja wrote: > Hi again, did you see my question about tests above? :) Sorry. I'm thinking of how to test this prepareToSuspend. I think, it is necessary to check whether really the resource is released (i.e. download and parsing are stopped) or not. And also we need to check whether the resource is correctly reloaded when resumed or not. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
