Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
866273005:
Teach ScriptRunner how to yield and post on loading task queue (Closed)
|
Created:
5 years, 10 months ago by alex clarke (OOO till 29th) Modified:
5 years, 9 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, dglazkov+blink, eae+blinkwatch, rwlbuis, oilpan-reviews Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionTeach ScriptRunner how to yield
Previously ScriptRunner could execute an arbitrary number of scripts
leading to janks. Now ScriptRunner polls shouldYieldForHighPriorityWork
before running each script.
In addition the task to execute the scripts is now posted on the
loading task queue.
BUG=456777
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190648
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190766
Patch Set 1 #Patch Set 2 : Post on loading task queue #Patch Set 3 : Dont run scripts that are not ready #
Total comments: 10
Patch Set 4 : Unit tests! #Patch Set 5 : Needed to shutdown the sheduler owned by MockPlatform in TearDown() #
Total comments: 11
Patch Set 6 : Sami's comments #Patch Set 7 : Fix spelling mistake #
Total comments: 19
Patch Set 8 : Responding to feedback re task cancellation #
Total comments: 4
Patch Set 9 : Make executeScripts() always run at least one script #Patch Set 10 : Really fix assert on windows #
Total comments: 1
Patch Set 11 : Oilpan leak fix #Patch Set 12 : Rebase #Patch Set 13 : Rebase try 2 #
Messages
Total messages: 64 (21 generated)
alexclarke@chromium.org changed reviewers: + skyostil@chromium.org
Added some comments and kicked off some blink tryjobs. https://codereview.chromium.org/866273005/diff/40001/Source/core/dom/ScriptRu... File Source/core/dom/ScriptRunner.cpp (left): https://codereview.chromium.org/866273005/diff/40001/Source/core/dom/ScriptRu... Source/core/dom/ScriptRunner.cpp:137: scriptLoaders.swap(m_scriptsToExecuteSoon); Should we still take a snapshot of the pending scripts here? Otherwise we may end up spinning here for much longer if shouldYield never ends returning true and new scripts keep coming in. Also I don't think the ordering is preserved if either m_scriptsToExecuteSoon or m_scriptsToExecuteInOrder get modified inside the loop. https://codereview.chromium.org/866273005/diff/40001/Source/core/dom/ScriptRu... File Source/core/dom/ScriptRunner.cpp (right): https://codereview.chromium.org/866273005/diff/40001/Source/core/dom/ScriptRu... Source/core/dom/ScriptRunner.cpp:88: Scheduler::shared()->postLoadingTask(FROM_HERE, m_executeScriptsTaskFactory.task()); Should we check whether we already have a callback posted? I think the timer ends up doing that. https://codereview.chromium.org/866273005/diff/40001/Source/core/dom/ScriptRu... Source/core/dom/ScriptRunner.cpp:104: Scheduler::shared()->postLoadingTask(FROM_HERE, m_executeScriptsTaskFactory.task()); Ditto. https://codereview.chromium.org/866273005/diff/40001/Source/core/dom/ScriptRu... File Source/core/dom/ScriptRunner.h (right): https://codereview.chromium.org/866273005/diff/40001/Source/core/dom/ScriptRu... Source/core/dom/ScriptRunner.h:71: Deque<RawPtrWillBeMember<ScriptLoader>> m_scriptsToExecuteInOrder; Should this be WillBeHeapDeque? https://codereview.chromium.org/866273005/diff/40001/Source/core/dom/ScriptRu... Source/core/dom/ScriptRunner.h:73: Deque<RawPtrWillBeMember<ScriptLoader>> m_scriptsToExecuteSoon; Ditto.
Patchset #5 (id:80001) has been deleted
Patchset #5 (id:100001) has been deleted
PTAL https://codereview.chromium.org/866273005/diff/40001/Source/core/dom/ScriptRu... File Source/core/dom/ScriptRunner.cpp (left): https://codereview.chromium.org/866273005/diff/40001/Source/core/dom/ScriptRu... Source/core/dom/ScriptRunner.cpp:137: scriptLoaders.swap(m_scriptsToExecuteSoon); On 2015/02/10 18:40:01, Sami wrote: > Should we still take a snapshot of the pending scripts here? Otherwise we may > end up spinning here for much longer if shouldYield never ends returning true > and new scripts keep coming in. Good spot, that is a problem. > Also I don't think the ordering is preserved if > either m_scriptsToExecuteSoon or m_scriptsToExecuteInOrder get modified inside > the loop. m_scriptsToExecuteSoon and m_scriptsToExecuteInOrder are only ever appended to. As long as we don't execute too many scripts we're good. https://codereview.chromium.org/866273005/diff/40001/Source/core/dom/ScriptRu... File Source/core/dom/ScriptRunner.cpp (right): https://codereview.chromium.org/866273005/diff/40001/Source/core/dom/ScriptRu... Source/core/dom/ScriptRunner.cpp:88: Scheduler::shared()->postLoadingTask(FROM_HERE, m_executeScriptsTaskFactory.task()); On 2015/02/10 18:40:01, Sami wrote: > Should we check whether we already have a callback posted? I think the timer > ends up doing that. Both m_timer.startOneShot(0, FROM_HERE); and Scheduler::shared()->postLoadingTask(FROM_HERE, m_executeScriptsTaskFactory.task()); cancel any previously posted task. https://codereview.chromium.org/866273005/diff/40001/Source/core/dom/ScriptRu... Source/core/dom/ScriptRunner.cpp:104: Scheduler::shared()->postLoadingTask(FROM_HERE, m_executeScriptsTaskFactory.task()); On 2015/02/10 18:40:01, Sami wrote: > Ditto. Acknowledged. https://codereview.chromium.org/866273005/diff/40001/Source/core/dom/ScriptRu... File Source/core/dom/ScriptRunner.h (right): https://codereview.chromium.org/866273005/diff/40001/Source/core/dom/ScriptRu... Source/core/dom/ScriptRunner.h:71: Deque<RawPtrWillBeMember<ScriptLoader>> m_scriptsToExecuteInOrder; On 2015/02/10 18:40:01, Sami wrote: > Should this be WillBeHeapDeque? I suppose so. I have to admit I'm not sure of the significance of the heap allocation. https://codereview.chromium.org/866273005/diff/40001/Source/core/dom/ScriptRu... Source/core/dom/ScriptRunner.h:73: Deque<RawPtrWillBeMember<ScriptLoader>> m_scriptsToExecuteSoon; On 2015/02/10 18:40:01, Sami wrote: > Ditto. Done.
PTAL
Looks great. Thanks for adding the really comprehensive tests. Is it easy to check the (relevant) tests against the old implementation to make sure it has the same behavior? https://codereview.chromium.org/866273005/diff/120001/Source/core/dom/ScriptR... File Source/core/dom/ScriptRunner.cpp (right): https://codereview.chromium.org/866273005/diff/120001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunner.cpp:104: Scheduler::shared()->postLoadingTask(FROM_HERE, m_executeScriptsTaskFactory.task()); Like we talked about, let's rename |task| to something that makes it more obvious it cancels the previous post. https://codereview.chromium.org/866273005/diff/120001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunner.cpp:162: // New scripts are always appened to m_scriptsToExecuteSoon and m_scriptsToExecuteInOrder (never prepended) typo: appended https://codereview.chromium.org/866273005/diff/120001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunner.cpp:168: if (Scheduler::shared()->shouldYieldForHighPriorityWork()) { Let's add a test that yields at every point and checks the execution order doesn't change. https://codereview.chromium.org/866273005/diff/120001/Source/core/dom/ScriptR... File Source/core/dom/ScriptRunnerTest.cpp (right): https://codereview.chromium.org/866273005/diff/120001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunnerTest.cpp:205: // Make sure the async scripts where run before the in-order ones. typo: were
https://codereview.chromium.org/866273005/diff/120001/Source/core/dom/ScriptR... File Source/core/dom/ScriptRunner.cpp (right): https://codereview.chromium.org/866273005/diff/120001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunner.cpp:168: if (Scheduler::shared()->shouldYieldForHighPriorityWork()) { On 2015/02/19 12:52:13, Sami wrote: > Let's add a test that yields at every point and checks the execution order > doesn't change. Do we have different behavior in this scenario? 1. soon += [1] 2. order += [2] 3. executeScript yields 4. soon += [3] 5. executeScript will do: [1, 3, 2]. Previously we'd have run [1, 2, 3]. I have no idea if this is a problem or not.
PTAL https://codereview.chromium.org/866273005/diff/120001/Source/core/dom/ScriptR... File Source/core/dom/ScriptRunner.cpp (right): https://codereview.chromium.org/866273005/diff/120001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunner.cpp:104: Scheduler::shared()->postLoadingTask(FROM_HERE, m_executeScriptsTaskFactory.task()); On 2015/02/19 12:52:13, Sami wrote: > Like we talked about, let's rename |task| to something that makes it more > obvious it cancels the previous post. I'd prefer to do this in a follow up patch. I'll send that out today. https://codereview.chromium.org/866273005/diff/120001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunner.cpp:162: // New scripts are always appened to m_scriptsToExecuteSoon and m_scriptsToExecuteInOrder (never prepended) On 2015/02/19 12:52:14, Sami wrote: > typo: appended Done. https://codereview.chromium.org/866273005/diff/120001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunner.cpp:168: if (Scheduler::shared()->shouldYieldForHighPriorityWork()) { On 2015/02/19 13:40:00, Sami wrote: > On 2015/02/19 12:52:13, Sami wrote: > > Let's add a test that yields at every point and checks the execution order > > doesn't change. > > Do we have different behavior in this scenario? > > 1. soon += [1] > 2. order += [2] > 3. executeScript yields > 4. soon += [3] > 5. executeScript will do: [1, 3, 2]. Previously we'd have run [1, 2, 3]. > > I have no idea if this is a problem or not. Yes that does appear to be the case, and I think it's OK. From what I can tell, the spec only guarantees the order of execution for the in-order scripts. https://codereview.chromium.org/866273005/diff/120001/Source/core/dom/ScriptR... File Source/core/dom/ScriptRunnerTest.cpp (right): https://codereview.chromium.org/866273005/diff/120001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunnerTest.cpp:205: // Make sure the async scripts where run before the in-order ones. On 2015/02/19 12:52:14, Sami wrote: > typo: were Done.
lgtm. https://codereview.chromium.org/866273005/diff/120001/Source/core/dom/ScriptR... File Source/core/dom/ScriptRunnerTest.cpp (right): https://codereview.chromium.org/866273005/diff/120001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunnerTest.cpp:205: // Make sure the async scripts where run before the in-order ones. On 2015/02/19 15:11:33, alexclarke1 wrote: > On 2015/02/19 12:52:14, Sami wrote: > > typo: were > > Done. Missed this one?
New patchsets have been uploaded after l-g-t-m from skyostil@chromium.org
alexclarke@chromium.org changed reviewers: + marja@chromium.org, sigbjornf@opera.com
+marja & sof since you seem the most knowledgeable reviewers for this class. The motivation for this patch is we've seen traces where the the script runner has taken well over 100ms on android because it was running dozens of scripts. Teaching this class to yield should reduce jank. https://codereview.chromium.org/866273005/diff/120001/Source/core/dom/ScriptR... File Source/core/dom/ScriptRunnerTest.cpp (right): https://codereview.chromium.org/866273005/diff/120001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunnerTest.cpp:205: // Make sure the async scripts where run before the in-order ones. On 2015/02/19 16:25:53, Sami wrote: > On 2015/02/19 15:11:33, alexclarke1 wrote: > > On 2015/02/19 12:52:14, Sami wrote: > > > typo: were > > > > Done. > > Missed this one? Done.
Good work, just some details noticed. https://codereview.chromium.org/866273005/diff/160001/Source/core/dom/ScriptR... File Source/core/dom/ScriptRunner.cpp (right): https://codereview.chromium.org/866273005/diff/160001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunner.cpp:171: if (Scheduler::shared()->shouldYieldForHighPriorityWork()) { Add a private helper method to avoid the repetition of these details, if (shouldYieldForHighPriorityWork()) return; https://codereview.chromium.org/866273005/diff/160001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunner.cpp:180: if (!m_scriptsToExecuteInOrder.first()->isReady()) { Let's add an assert about non-emptiness, just in case. https://codereview.chromium.org/866273005/diff/160001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunner.cpp:182: } Redundant braces. https://codereview.chromium.org/866273005/diff/160001/Source/core/dom/ScriptR... File Source/core/dom/ScriptRunnerTest.cpp (right): https://codereview.chromium.org/866273005/diff/160001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunnerTest.cpp:10: #include "platform/scheduler/Scheduler.h" Add #include "platform/heap/Handle.h" round about here. https://codereview.chromium.org/866273005/diff/160001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunnerTest.cpp:105: RefPtr<Document> m_document; Could you make this RefPtrWillBePersistent<> instead? https://codereview.chromium.org/866273005/diff/160001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunnerTest.cpp:106: RefPtr<Element> m_element; RefPtrWillBePersistent<> https://codereview.chromium.org/866273005/diff/160001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunnerTest.cpp:107: OwnPtr<ScriptRunner> m_scriptRunner; OwnPtrWillBePersistent<>
https://codereview.chromium.org/866273005/diff/160001/Source/core/dom/ScriptR... File Source/core/dom/ScriptRunner.cpp (right): https://codereview.chromium.org/866273005/diff/160001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunner.cpp:105: Scheduler::shared()->postLoadingTask(FROM_HERE, m_executeScriptsTaskFactory.task()); Hmm, so, does this mean that if the task is already in the task queue, we take it out and post another one which goes to the end of the task queue? Why is that desired behavior? Shouldn't we rather not post a task if there's one in flight already? https://codereview.chromium.org/866273005/diff/160001/Source/core/dom/ScriptR... File Source/core/dom/ScriptRunnerTest.cpp (right): https://codereview.chromium.org/866273005/diff/160001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunnerTest.cpp:342: TEST_F(ScriptRunnerTest, ShouldYield_AsyncScripts) Nice!
lgtm though, after you address sof's comments. I'm not sure about the task cancelling behavior either way, I don't have a strong argument why that behavior would be un-desired either - though, it delays script execution in some cases, but not sure what are its practical implications and whether it makes any sense to add more code to do it otherwise.
New patchsets have been uploaded after l-g-t-m from marja@chromium.org
I added a helper postTaskIfOneIsNotAlreadyInFlight() which addresses marja@'s concern. PTAL https://codereview.chromium.org/866273005/diff/160001/Source/core/dom/ScriptR... File Source/core/dom/ScriptRunner.cpp (right): https://codereview.chromium.org/866273005/diff/160001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunner.cpp:105: Scheduler::shared()->postLoadingTask(FROM_HERE, m_executeScriptsTaskFactory.task()); On 2015/02/20 09:34:37, marja wrote: > Hmm, so, does this mean that if the task is already in the task queue, we take > it out and post another one which goes to the end of the task queue? Why is that > desired behavior? Shouldn't we rather not post a task if there's one in flight > already? Good point, this lead to some discussion in the office. The actual behavior of the Blink Timer Heap is quite complicated. In the case where it's lightly loaded, calling m_timer.startOneShot(0, ...) at time T+0 and T+1 will cause the underlying chromium timer to get cancelled and another task to get posted to call the shared platform timers (i.e. identical behavior to this). If the Blink Timer heap is heavily loaded, it's quite likely that calling m_timer.startOneShot(0, ...) the second time won't change the first entry in the timer heap. Anyway I think it's probably best to use the in-flight task if one exists because in a (likely theoretical) scenario where a very large number of notifyScriptReady calls happen we might get unreasonable delay. As a side note I look forward to the day when we can delete the Blink Timer Heap, the behavior of that code is far too difficult to reason about. https://codereview.chromium.org/866273005/diff/160001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunner.cpp:171: if (Scheduler::shared()->shouldYieldForHighPriorityWork()) { On 2015/02/20 07:48:50, sof wrote: > Add a private helper method to avoid the repetition of these details, > > if (shouldYieldForHighPriorityWork()) > return; Done. https://codereview.chromium.org/866273005/diff/160001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunner.cpp:180: if (!m_scriptsToExecuteInOrder.first()->isReady()) { On 2015/02/20 07:48:50, sof wrote: > Let's add an assert about non-emptiness, just in case. Done. https://codereview.chromium.org/866273005/diff/160001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunner.cpp:182: } On 2015/02/20 07:48:50, sof wrote: > Redundant braces. Done. https://codereview.chromium.org/866273005/diff/160001/Source/core/dom/ScriptR... File Source/core/dom/ScriptRunnerTest.cpp (right): https://codereview.chromium.org/866273005/diff/160001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunnerTest.cpp:10: #include "platform/scheduler/Scheduler.h" On 2015/02/20 07:48:50, sof wrote: > Add #include "platform/heap/Handle.h" round about here. Done. https://codereview.chromium.org/866273005/diff/160001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunnerTest.cpp:105: RefPtr<Document> m_document; On 2015/02/20 07:48:50, sof wrote: > Could you make this RefPtrWillBePersistent<> instead? Done. https://codereview.chromium.org/866273005/diff/160001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunnerTest.cpp:106: RefPtr<Element> m_element; On 2015/02/20 07:48:50, sof wrote: > RefPtrWillBePersistent<> Done. https://codereview.chromium.org/866273005/diff/160001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunnerTest.cpp:107: OwnPtr<ScriptRunner> m_scriptRunner; On 2015/02/20 07:48:50, sof wrote: > OwnPtrWillBePersistent<> Done. https://codereview.chromium.org/866273005/diff/160001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunnerTest.cpp:107: OwnPtr<ScriptRunner> m_scriptRunner; On 2015/02/20 07:48:50, sof wrote: > OwnPtrWillBePersistent<> Done. https://codereview.chromium.org/866273005/diff/160001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunnerTest.cpp:342: TEST_F(ScriptRunnerTest, ShouldYield_AsyncScripts) On 2015/02/20 09:34:37, marja wrote: > Nice! :)
still lgtm w/ nits + comment https://codereview.chromium.org/866273005/diff/180001/Source/core/dom/ScriptR... File Source/core/dom/ScriptRunner.cpp (right): https://codereview.chromium.org/866273005/diff/180001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunner.cpp:170: if (shouldYieldForHighPriorityWork()) Another behavioral consideration: when this executeScripts() is called, should we expect it to execute at least 1 script before yielding? Why / why not? (I can think of reasons both ways...) https://codereview.chromium.org/866273005/diff/180001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunner.cpp:186: bool ScriptRunner::shouldYieldForHighPriorityWork() Nit: this function name is confusing, I'd just expect a function called shouldFoo() just return boolean and *not* do any other work such as scheduling a task. Idk what would be a better name, yieldForHighPriorityWork maybe? (Though, that function cannot just yield on behalf of the caller, so it's not perfect either.)
lgtm
New patchsets have been uploaded after l-g-t-m from marja@chromium.org,sigbjornf@opera.com
https://codereview.chromium.org/866273005/diff/180001/Source/core/dom/ScriptR... File Source/core/dom/ScriptRunner.cpp (right): https://codereview.chromium.org/866273005/diff/180001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunner.cpp:170: if (shouldYieldForHighPriorityWork()) On 2015/02/20 12:06:52, marja wrote: > Another behavioral consideration: when this executeScripts() is called, should > we expect it to execute at least 1 script before yielding? Why / why not? (I can > think of reasons both ways...) The Scheduler shouldn't run a task from the LoadingTaskQueue when shouldYieldForHighPriorityWork is true. So we might as well move the yield to after the execute. It's clearer that something will be done. https://codereview.chromium.org/866273005/diff/180001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunner.cpp:186: bool ScriptRunner::shouldYieldForHighPriorityWork() On 2015/02/20 12:06:52, marja wrote: > Nit: this function name is confusing, I'd just expect a function called > shouldFoo() just return boolean and *not* do any other work such as scheduling a > task. > > Idk what would be a better name, yieldForHighPriorityWork maybe? (Though, that > function cannot just yield on behalf of the caller, so it's not perfect either.) Lets go with yieldForHighPriorityWork.
Still lgtm.
The CQ bit was checked by alexclarke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/866273005/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_dbg on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/4...)
New patchsets have been uploaded after l-g-t-m from skyostil@chromium.org
The CQ bit was checked by alexclarke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/866273005/220001
Patchset #10 (id:220001) has been deleted
The CQ bit was checked by alexclarke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/866273005/240001
The CQ bit was unchecked by alexclarke@chromium.org
The CQ bit was checked by alexclarke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/866273005/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/51723)
Patchset #10 (id:240001) has been deleted
The CQ bit was checked by alexclarke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/866273005/260001
Message was sent while issue was closed.
Committed patchset #10 (id:260001) as https://src.chromium.org/viewvc/blink?view=rev&revision=190648
Message was sent while issue was closed.
Causes some leaks on the Oilpan leak bots (5k of them) + webkit unit tests failures, http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:260001) has been created in https://codereview.chromium.org/936493003/ by alexclarke@chromium.org. The reason for reverting is: Looks like this was causing oilpan leaks.
Message was sent while issue was closed.
https://codereview.chromium.org/866273005/diff/260001/Source/core/dom/ScriptR... File Source/core/dom/ScriptRunner.cpp (right): https://codereview.chromium.org/866273005/diff/260001/Source/core/dom/ScriptR... Source/core/dom/ScriptRunner.cpp:40: , m_executeScriptsTaskFactory(WTF::bind(&ScriptRunner::executeScripts, this)) This creates a Persistent back reference to ScriptRunner, preventing it (and everything it refers to) from being GCed.
Message was sent while issue was closed.
On 2015/02/23 14:49:09, sof wrote: > https://codereview.chromium.org/866273005/diff/260001/Source/core/dom/ScriptR... > File Source/core/dom/ScriptRunner.cpp (right): > > https://codereview.chromium.org/866273005/diff/260001/Source/core/dom/ScriptR... > Source/core/dom/ScriptRunner.cpp:40: , > m_executeScriptsTaskFactory(WTF::bind(&ScriptRunner::executeScripts, this)) > This creates a Persistent back reference to ScriptRunner, preventing it (and > everything it refers to) from being GCed. The Persistent subtly getting in the way of what only needs to be a raw pointer reference from the closure, as CancellableTaskFactory is owned by ScriptRunner. With the specializations being as they are for PointerParamStorageTraits<>, using RawPtrWillBeMember<ScriptRunner>(this) instead would work around it, but that's terribly hacky. Moving Function<> to the heap seems far fetched, so not sure what to suggest. Other oilpan-reviews members may have suggestions..?
Message was sent while issue was closed.
haraken@chromium.org changed reviewers: + haraken@chromium.org, tkent@chromium.org
Message was sent while issue was closed.
tkent-san might have an idea since he designed cross-thread tasks in oilpan.
Message was sent while issue was closed.
We shouldn't revert a CL for Oilpan-only failures. You may re-land this as is.
Oilpan-team is responsible to fix the Oilpan failures.
In this case, ownership is very clear and we may use a raw pointer in the
Closure.
Probably adding the following to ScriptRunner.cpp fixes the problem.
namespace WTF {
template<blink::ScriptRunner>
struct ParamStorageTraits<blink::ScriptRunner*> : public
PointerParamStorageTraits<blink::ScriptRunner*, false> {
};
}
Message was sent while issue was closed.
On 2015/02/24 03:38:20, tkent wrote:
> We shouldn't revert a CL for Oilpan-only failures. You may re-land this as
is.
> Oilpan-team is responsible to fix the Oilpan failures.
>
> In this case, ownership is very clear and we may use a raw pointer in the
> Closure.
> Probably adding the following to ScriptRunner.cpp fixes the problem.
>
> namespace WTF {
> template<blink::ScriptRunner>
> struct ParamStorageTraits<blink::ScriptRunner*> : public
> PointerParamStorageTraits<blink::ScriptRunner*, false> {
> };
> }
Sounds like a plan :) If alexclarke@ wants to go ahead and reland as is, please
do & we'll tidy up as suggested.
Message was sent while issue was closed.
Re: Oilpan, I'm slightly worried about Oilpan-compatible programming patterns and their understandability. It seems that the Oilpan fixes needed are often non-trivial and hard to understand for non-Oilpan engineers. What's up with that? Is it because stuff is in progress? (In some CLs, it has been because some but not all of the objects in question are in the Oilpan heap, but in this CL that's not the case afaics.) Is it because non-Oilpan engineers have not yet been sufficiently educated on how to program w/ Oilpan?
Message was sent while issue was closed.
On 2015/02/24 08:31:50, marja wrote: > Re: Oilpan, I'm slightly worried about Oilpan-compatible programming patterns > and their understandability. It seems that the Oilpan fixes needed are often > non-trivial and hard to understand for non-Oilpan engineers. What's up with > that? Is it because stuff is in progress? (In some CLs, it has been because some > but not all of the objects in question are in the Oilpan heap, but in this CL > that's not the case afaics.) Is it because non-Oilpan engineers have not yet > been sufficiently educated on how to program w/ Oilpan? That's a valid concern and I understand it. We have to improve the programmability of Oilpan and are doing that :) We'll drop WillBe types once we ship Oilpan, which will simplify things a bit.
Message was sent while issue was closed.
On 2015/02/24 08:40:59, haraken wrote: > On 2015/02/24 08:31:50, marja wrote: > > Re: Oilpan, I'm slightly worried about Oilpan-compatible programming patterns > > and their understandability. It seems that the Oilpan fixes needed are often > > non-trivial and hard to understand for non-Oilpan engineers. What's up with > > that? Is it because stuff is in progress? (In some CLs, it has been because > some > > but not all of the objects in question are in the Oilpan heap, but in this CL > > that's not the case afaics.) Is it because non-Oilpan engineers have not yet > > been sufficiently educated on how to program w/ Oilpan? > > That's a valid concern and I understand it. We have to improve the > programmability of Oilpan and are doing that :) > > We'll drop WillBe types once we ship Oilpan, which will simplify things a bit. Lifetime and ownership aren't obvious issues to handle outside Oilpan either; relevant to not lose sight of that here, imho.
Message was sent while issue was closed.
Yes - in addition, Oilpan is supposed to make it easier. Equally hard is not good enough :)
Message was sent while issue was closed.
On 2015/02/24 03:38:20, tkent wrote:
> We shouldn't revert a CL for Oilpan-only failures. You may re-land this as
is.
> Oilpan-team is responsible to fix the Oilpan failures.
>
> In this case, ownership is very clear and we may use a raw pointer in the
> Closure.
> Probably adding the following to ScriptRunner.cpp fixes the problem.
>
> namespace WTF {
> template<blink::ScriptRunner>
> struct ParamStorageTraits<blink::ScriptRunner*> : public
> PointerParamStorageTraits<blink::ScriptRunner*, false> {
> };
> }
I tried that but I get compile errors:
../../third_party/WebKit/Source/core/dom/ScriptRunner.cpp:38:29: error: a
non-type template parameter cannot have type 'blink::ScriptRunner'
template<blink::ScriptRunner>
^
../../third_party/WebKit/Source/core/dom/ScriptRunner.cpp:39:8: error: partial
specialization of 'ParamStorageTraits' does not use any of its template
parameters
struct ParamStorageTraits<blink::ScriptRunner*> : public
PointerParamStorageTraits<blink::ScriptRunner*, false> {
Message was sent while issue was closed.
On 2015/02/24 14:20:02, alexclarke1 wrote:
> On 2015/02/24 03:38:20, tkent wrote:
> > We shouldn't revert a CL for Oilpan-only failures. You may re-land this as
> is.
> > Oilpan-team is responsible to fix the Oilpan failures.
> >
> > In this case, ownership is very clear and we may use a raw pointer in the
> > Closure.
> > Probably adding the following to ScriptRunner.cpp fixes the problem.
> >
> > namespace WTF {
> > template<blink::ScriptRunner>
> > struct ParamStorageTraits<blink::ScriptRunner*> : public
> > PointerParamStorageTraits<blink::ScriptRunner*, false> {
> > };
> > }
>
> I tried that but I get compile errors:
>
> ../../third_party/WebKit/Source/core/dom/ScriptRunner.cpp:38:29: error: a
> non-type template parameter cannot have type 'blink::ScriptRunner'
> template<blink::ScriptRunner>
> ^
> ../../third_party/WebKit/Source/core/dom/ScriptRunner.cpp:39:8: error: partial
> specialization of 'ParamStorageTraits' does not use any of its template
> parameters
> struct ParamStorageTraits<blink::ScriptRunner*> : public
> PointerParamStorageTraits<blink::ScriptRunner*, false> {
Yes, that's not well-formed - use "template<>" instead.
On 2015/02/24 14:28:45, sof wrote:
> On 2015/02/24 14:20:02, alexclarke1 wrote:
> > On 2015/02/24 03:38:20, tkent wrote:
> > > We shouldn't revert a CL for Oilpan-only failures. You may re-land this
as
> > is.
> > > Oilpan-team is responsible to fix the Oilpan failures.
> > >
> > > In this case, ownership is very clear and we may use a raw pointer in the
> > > Closure.
> > > Probably adding the following to ScriptRunner.cpp fixes the problem.
> > >
> > > namespace WTF {
> > > template<blink::ScriptRunner>
> > > struct ParamStorageTraits<blink::ScriptRunner*> : public
> > > PointerParamStorageTraits<blink::ScriptRunner*, false> {
> > > };
> > > }
> >
> > I tried that but I get compile errors:
> >
> > ../../third_party/WebKit/Source/core/dom/ScriptRunner.cpp:38:29: error: a
> > non-type template parameter cannot have type 'blink::ScriptRunner'
> > template<blink::ScriptRunner>
> > ^
> > ../../third_party/WebKit/Source/core/dom/ScriptRunner.cpp:39:8: error:
partial
> > specialization of 'ParamStorageTraits' does not use any of its template
> > parameters
> > struct ParamStorageTraits<blink::ScriptRunner*> : public
> > PointerParamStorageTraits<blink::ScriptRunner*, false> {
>
> Yes, that's not well-formed - use "template<>" instead.
Thanks, that appears to make the leaks go away when I run locally with
enable_oilpan=1. Trying a new patchset on the bots.
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com, skyostil@chromium.org, marja@chromium.org Link to the patchset: https://codereview.chromium.org/866273005/#ps320001 (title: "Rebase try 2")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/866273005/320001
On 2015/02/24 15:00:04, alexclarke1 wrote:
> On 2015/02/24 14:28:45, sof wrote:
> > On 2015/02/24 14:20:02, alexclarke1 wrote:
> > > On 2015/02/24 03:38:20, tkent wrote:
> > > > We shouldn't revert a CL for Oilpan-only failures. You may re-land this
> as
> > > is.
> > > > Oilpan-team is responsible to fix the Oilpan failures.
> > > >
> > > > In this case, ownership is very clear and we may use a raw pointer in
the
> > > > Closure.
> > > > Probably adding the following to ScriptRunner.cpp fixes the problem.
> > > >
> > > > namespace WTF {
> > > > template<blink::ScriptRunner>
> > > > struct ParamStorageTraits<blink::ScriptRunner*> : public
> > > > PointerParamStorageTraits<blink::ScriptRunner*, false> {
> > > > };
> > > > }
> > >
> > > I tried that but I get compile errors:
> > >
> > > ../../third_party/WebKit/Source/core/dom/ScriptRunner.cpp:38:29: error: a
> > > non-type template parameter cannot have type 'blink::ScriptRunner'
> > > template<blink::ScriptRunner>
> > > ^
> > > ../../third_party/WebKit/Source/core/dom/ScriptRunner.cpp:39:8: error:
> partial
> > > specialization of 'ParamStorageTraits' does not use any of its template
> > > parameters
> > > struct ParamStorageTraits<blink::ScriptRunner*> : public
> > > PointerParamStorageTraits<blink::ScriptRunner*, false> {
> >
> > Yes, that's not well-formed - use "template<>" instead.
>
> Thanks, that appears to make the leaks go away when I run locally with
> enable_oilpan=1. Trying a new patchset on the bots.
Latest patchset confirmed locally to be fine wrt leaks w/ Oilpan enabled. Thanks
again for taking care of it directly.
Message was sent while issue was closed.
Committed patchset #13 (id:320001) as https://src.chromium.org/viewvc/blink?view=rev&revision=190766
Message was sent while issue was closed.
The newly-added test ScriptRunnerTest.QueueSingleScript_Async fails on the first run (and succeeds on retry). https://crbug.com/463807 Could you take a look? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
