Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1749073002:
Do V8 GC ASAP if system memory is pressured (Closed)
|
Created:
4 years, 9 months ago by hong.zheng Modified:
4 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, esprehn, gavinp+memory_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, vmpstr+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDo V8 GC ASAP if system memory is pressured
Currently after receiving memory pressure notification, Chrome posts
asynchronous task to renderer main thread to trigger V8 GC. If renderer
main thread is busy, purging memory task will be postponed running,
which increases OOM risk.
The CL can send memory pressure notification to renderer main thread
synchronously and trigger V8 GC while V8 is in the middle of executing
JavaScript.
The CL depends on CL 1813963002
BUG=590975
Committed: https://crrev.com/f80422e52658fd87d4159bfeeddd703bae1d4180
Cr-Commit-Position: refs/heads/master@{#391167}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Use ObserverList for synchronous notification and use the interrupt mechanism to send memory pressu… #
Total comments: 3
Patch Set 3 : Standardize variable name and move three statics into a struct in MemoryPressureLister class. In ad… #Patch Set 4 : based on CL 1813963002 #
Total comments: 18
Patch Set 5 : add global listener and send notification to worker earlier #Patch Set 6 : #
Total comments: 2
Patch Set 7 : send synchronous notification to worker thread #
Total comments: 10
Patch Set 8 : transfer memory pressure notification to worker thread isolates #
Total comments: 8
Patch Set 9 : update in WorkerBackingThread #
Total comments: 8
Patch Set 10 : address comments #
Total comments: 6
Patch Set 11 : address comments #
Total comments: 8
Patch Set 12 : extract MemoryPressureObserver from MemoryPressureListener #Patch Set 13 : rebase #
Total comments: 1
Messages
Total messages: 78 (20 generated)
Description was changed from ========== Do V8 GC ASAP if system memory is pressured Currently after receiving memory pressure notification, Chrome posts asynchronous task to renderer main thread to trigger V8 GC. If renderer main thread is busy, purging memory task will be postponed running, which increases OOM risk. The CL can send memory pressure notification to renderer main thread synchronously and trigger V8 GC while V8 is in the middle of executing JavaScript. BUG=590975 ========== to ========== Do V8 GC ASAP if system memory is pressured Currently after receiving memory pressure notification, Chrome posts asynchronous task to renderer main thread to trigger V8 GC. If renderer main thread is busy, purging memory task will be postponed running, which increases OOM risk. The CL can send memory pressure notification to renderer main thread synchronously and trigger V8 GC while V8 is in the middle of executing JavaScript. BUG=590975 ==========
hong.zheng@intel.com changed reviewers: + esprehn@chromium.org, mark@chromium.org
On 2016/03/01 10:26:48, hong.zheng wrote: > mailto:hong.zheng@intel.com changed reviewers: > + mailto:esprehn@chromium.org, mailto:mark@chromium.org PTAL
https://codereview.chromium.org/1749073002/diff/1/base/observer_list_threadsa... File base/observer_list_threadsafe.h (right): https://codereview.chromium.org/1749073002/diff/1/base/observer_list_threadsa... base/observer_list_threadsafe.h:196: // Note, these calls are synchronous. …and happen on thread that calls SyncNotify, which does not match the expected semantics of ObserverListThreadSafe. ObserverListThreadSafe callbacks run on the threads that registered the observers. The “synchronous” observer list implementation is ObserverList. If you need something synchronous and can accept the caveat that callbacks run on the thread that is calling Notify, not the threads that registered the observers, then you should be using ObserverList instead. If you need these semantics in a multi-threaded situation, use an ObserverList protected by suitable locking. Refer to the comments at the top of observer_list_threadsafe.h for more details.
esprehn@chromium.org changed reviewers: + jochen@chromium.org
https://codereview.chromium.org/1749073002/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1749073002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:2199: blink::mainThreadIsolate()->LowMemoryNotification(); Is this safe? You're using the LowMemoryNotification method on Isolate from the wrong thread, I don't think that's okay.
On 2016/03/01 15:21:20, Mark Mentovai wrote: > https://codereview.chromium.org/1749073002/diff/1/base/observer_list_threadsa... > File base/observer_list_threadsafe.h (right): > > https://codereview.chromium.org/1749073002/diff/1/base/observer_list_threadsa... > base/observer_list_threadsafe.h:196: // Note, these calls are synchronous. > …and happen on thread that calls SyncNotify, which does not match the expected > semantics of ObserverListThreadSafe. ObserverListThreadSafe callbacks run on the > threads that registered the observers. > > The “synchronous” observer list implementation is ObserverList. If you need > something synchronous and can accept the caveat that callbacks run on the thread > that is calling Notify, not the threads that registered the observers, then you > should be using ObserverList instead. If you need these semantics in a > multi-threaded situation, use an ObserverList protected by suitable locking. > > Refer to the comments at the top of observer_list_threadsafe.h for more details. Thanks Mark's advice, I will try it.
On 2016/03/01 22:15:07, esprehn wrote: > https://codereview.chromium.org/1749073002/diff/1/content/renderer/render_thr... > File content/renderer/render_thread_impl.cc (right): > > https://codereview.chromium.org/1749073002/diff/1/content/renderer/render_thr... > content/renderer/render_thread_impl.cc:2199: > blink::mainThreadIsolate()->LowMemoryNotification(); > Is this safe? You're using the LowMemoryNotification method on Isolate from the > wrong thread, I don't think that's okay. The RequestInterrupt is called in different thread. But the LowMemoryNotification() is called in V8 thread. Do you mean it is wrong?
jochen@chromium.org changed reviewers: + rmcilroy@chromium.org, ulan@chromium.org
+ross/ulan as I was discussing something similar with them In general, I'm very unhappy about those low memory notifications. Invoking one will freeze the process for >1s on mobile. The fact that we get into a memory pressure situation also means that the entire system has screwed up, so just invoking it earlier is not addressing the real problem here. https://codereview.chromium.org/1749073002/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1749073002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:2205: blink::mainThreadIsolate()->RequestInterrupt(&OnV8InterruptCallback, this); the interrupt callback must not reenter V8.
On 2016/03/02 07:59:38, jochen wrote: > +ross/ulan as I was discussing something similar with them > > In general, I'm very unhappy about those low memory notifications. Invoking one > will freeze the process for >1s on mobile. > I agree with Jochen that in real world LowMemoryNotification() often costs long time especially on low end Android device. Do we have other better method to purge V8 memory when memory is pressured? > The fact that we get into a memory pressure situation also means that the entire > system has screwed up, so just invoking it earlier is not addressing the real > problem here. > What causes memory pressure is complicated problem, which could be browser issue, but also could be web page issue. On low end device, memory pressure is difficult to avoid. So I think if we can purge memory ASAP when memory is pressured, we can reduce OOM risk and make Chrome more robust. > https://codereview.chromium.org/1749073002/diff/1/content/renderer/render_thr... > File content/renderer/render_thread_impl.cc (right): > > https://codereview.chromium.org/1749073002/diff/1/content/renderer/render_thr... > content/renderer/render_thread_impl.cc:2205: > blink::mainThreadIsolate()->RequestInterrupt(&OnV8InterruptCallback, this); > the interrupt callback must not reenter V8. Yes, I noticed the comments in V8.h. Because OnV8InterruptCallback will be called in StackGuard::HandleInterrupts and StackGuard::HandleInterrupts supports GC operation, like isolate_->heap()->HandleGCRequest(), so I use RequestInterrupt here. But I am a newbie in V8 and might not realize some issues, if RequestInterrupt can't be used here, could you please give some advice about how to trigger V8 GC during V8 execution? As the expert, your guidance and suggestion is very helpful to me, thanks in advance.
There was a question in bug discussion from Sami: > What's the user visible impact here, i.e., how much would servicing the notification sooner help? Does delaying the notification make it more likely that the tab is killed? I am also interested in the impact of the CL. If we are close to OOM, my intuition would be that forcing GC would free up memory for few seconds and then we are close to OOM again. If we constantly get memory pressure signals then this would result in huge lag due to forced GCs. I think we are choosing here between crashing or making Chrome unusably slow.
On 2016/03/02 09:12:08, ulan wrote: > There was a question in bug discussion from Sami: > > What's the user visible impact here, i.e., how much would servicing the > notification sooner help? Does delaying the notification make it more likely > that the tab is killed? > > I am also interested in the impact of the CL. If we are close to OOM, my > intuition would be that forcing GC would free up memory for few seconds and then > we are close to OOM again. If we constantly get memory pressure signals then > this would result in huge lag due to forced GCs. I think we are choosing here > between crashing or making Chrome unusably slow. About the impact, I agreed with what esprehn mentioned in the discussion "or that chrome itself is killed if we're backgrounded, or that chrome will have to kill other tabs since the render process which could have freed a bunch of memory was not able to process it." I collected some data with the CL and uploaded the result in BUG=590975. Through comparison with before data(https://groups.google.com/a/chromium.org/d/msg/project-trim/u_5DDPBXK74/MPebAZSrAwAJ) we can see, the CL can significantly reduce V8 GC lag. In addition, in current mechanism, LowMemoryNotification is triggered in OnMemoryPressure, which can be postponed long time(>1000ms). The CL only tries to do GC in advance, which can reduce OOM risk and not cause additional performance impact.
hi jochen, according to your comments, whether we can send a new InterruptFlag(MEMORY_PRESSURE_INTERRUPT, system memory pressure flag) to the V8, like API_INTERRUPT. In this way, we can send memory pressure notification to V8 synchronously and V8 can handle it in StackGuard::HandleInterrupts in appropriate time. This solution should also be able to gain the benefit of triggering V8 GC earlier when memory is pressured. How do you think?
On 2016/03/04 at 03:21:43, hong.zheng wrote: > hi jochen, > according to your comments, whether we can send a new InterruptFlag(MEMORY_PRESSURE_INTERRUPT, system memory pressure flag) to the V8, like API_INTERRUPT. In this way, we can send memory pressure notification to V8 synchronously and V8 can handle it in StackGuard::HandleInterrupts in appropriate time. This solution should also be able to gain the benefit of triggering V8 GC earlier when memory is pressured. How do you think? We're actually considering to not trigger three full GCs in a low-memory notification anymore, but instead use the low memory notification as a signal to GC similar to the background notification. Using the interrupt mechanism to be able to send them from any thread at any time might be a good idea nevertheless. Ulan, what do you think?
On 2016/03/04 09:40:29, jochen wrote: > On 2016/03/04 at 03:21:43, hong.zheng wrote: > > hi jochen, > > according to your comments, whether we can send a new > InterruptFlag(MEMORY_PRESSURE_INTERRUPT, system memory pressure flag) to the V8, > like API_INTERRUPT. In this way, we can send memory pressure notification to V8 > synchronously and V8 can handle it in StackGuard::HandleInterrupts in > appropriate time. This solution should also be able to gain the benefit of > triggering V8 GC earlier when memory is pressured. How do you think? > > We're actually considering to not trigger three full GCs in a low-memory > notification anymore, but instead use the low memory notification as a signal to > GC similar to the background notification. > > Using the interrupt mechanism to be able to send them from any thread at any > time might be a good idea nevertheless. Ulan, what do you think? Interrupt to notify V8 sgtm, but we need a mechanism to ensure that V8 is not interrupted too often. How often is memory pressure signal sent?
Description was changed from ========== Do V8 GC ASAP if system memory is pressured Currently after receiving memory pressure notification, Chrome posts asynchronous task to renderer main thread to trigger V8 GC. If renderer main thread is busy, purging memory task will be postponed running, which increases OOM risk. The CL can send memory pressure notification to renderer main thread synchronously and trigger V8 GC while V8 is in the middle of executing JavaScript. BUG=590975 ========== to ========== Do V8 GC ASAP if system memory is pressured Currently after receiving memory pressure notification, Chrome posts asynchronous task to renderer main thread to trigger V8 GC. If renderer main thread is busy, purging memory task will be postponed running, which increases OOM risk. The CL can send memory pressure notification to renderer main thread synchronously and trigger V8 GC while V8 is in the middle of executing JavaScript. The patch includes two CLs, one in Chromium and the other in V8 repo(https://codereview.chromium.org/1777883002/) BUG=590975 ==========
On 2016/03/04 09:49:02, ulan wrote: > On 2016/03/04 09:40:29, jochen wrote: > > On 2016/03/04 at 03:21:43, hong.zheng wrote: > > > hi jochen, > > > according to your comments, whether we can send a new > > InterruptFlag(MEMORY_PRESSURE_INTERRUPT, system memory pressure flag) to the > V8, > > like API_INTERRUPT. In this way, we can send memory pressure notification to > V8 > > synchronously and V8 can handle it in StackGuard::HandleInterrupts in > > appropriate time. This solution should also be able to gain the benefit of > > triggering V8 GC earlier when memory is pressured. How do you think? > > > > We're actually considering to not trigger three full GCs in a low-memory > > notification anymore, but instead use the low memory notification as a signal > to > > GC similar to the background notification. > > > > Using the interrupt mechanism to be able to send them from any thread at any > > time might be a good idea nevertheless. Ulan, what do you think? > > Interrupt to notify V8 sgtm, but we need a mechanism to ensure that V8 is not > interrupted too often. > How often is memory pressure signal sent? Different platforms have different sending memory pressure signal mechanisms. For example, on Android, the memory pressure signal is controlled by system. System monitors memory consumption and sends signal to application. On ChromeOS, Chrome checks memory status and notifies MemoryPressureListener memory pressure level on its own through MemoryPressureMonitor class. In my testing, I only catch one notification in about 30s duration(details, please refer to trace file in BUG=590975). But I think it will be perfect if V8 has a mechanism to avoid LowMemoryNotification too frequency.
PTAL. The CL of V8 is in https://codereview.chromium.org/1777883002/
https://codereview.chromium.org/1749073002/diff/20001/base/memory/memory_pres... File base/memory/memory_pressure_listener.cc (right): https://codereview.chromium.org/1749073002/diff/20001/base/memory/memory_pres... base/memory/memory_pressure_listener.cc:38: sync_observers_lock_ = LAZY_INSTANCE_INITIALIZER; Why the trailing underscore? There’s no trailing underscore on sync_observers. Why not use g_ naming like g_observers? https://codereview.chromium.org/1749073002/diff/20001/base/memory/memory_pres... base/memory/memory_pressure_listener.cc:38: sync_observers_lock_ = LAZY_INSTANCE_INITIALIZER; Rather than having three statics here, maybe you should move these statics to be member variables of MemoryPressureListener, and have a single static pointing to the live MemoryPressureListener here. https://codereview.chromium.org/1749073002/diff/20001/base/memory/memory_pres... File base/memory/memory_pressure_listener.h (right): https://codereview.chromium.org/1749073002/diff/20001/base/memory/memory_pres... base/memory/memory_pressure_listener.h:95: No blank line here.
Thanks Mark and Jochen for the comments. Patch Set 3 standardizes variable name and moves three statics into a struct in MemoryPressureListener. In addition, according to Jochen's comments(in V8 CL, https://codereview.chromium.org/1777883002/), add an interface to report memory pressure level to V8. PTAL.
Description was changed from ========== Do V8 GC ASAP if system memory is pressured Currently after receiving memory pressure notification, Chrome posts asynchronous task to renderer main thread to trigger V8 GC. If renderer main thread is busy, purging memory task will be postponed running, which increases OOM risk. The CL can send memory pressure notification to renderer main thread synchronously and trigger V8 GC while V8 is in the middle of executing JavaScript. The patch includes two CLs, one in Chromium and the other in V8 repo(https://codereview.chromium.org/1777883002/) BUG=590975 ========== to ========== Do V8 GC ASAP if system memory is pressured Currently after receiving memory pressure notification, Chrome posts asynchronous task to renderer main thread to trigger V8 GC. If renderer main thread is busy, purging memory task will be postponed running, which increases OOM risk. The CL can send memory pressure notification to renderer main thread synchronously and trigger V8 GC while V8 is in the middle of executing JavaScript. The CL depends on CL 1813963002 BUG=590975 ==========
PTAL. Depend on CL 1813963002
On 2016/03/25 03:14:29, hong.zheng wrote: > PTAL. Depend on CL 1813963002 The BUG=590975 has been updated with the latest test results
https://codereview.chromium.org/1749073002/diff/60001/base/memory/memory_pres... File base/memory/memory_pressure_listener.h (right): https://codereview.chromium.org/1749073002/diff/60001/base/memory/memory_pres... base/memory/memory_pressure_listener.h:50: // ObserverListThreadSafe is RefCountedThreadSafe, this traits is needed Why does this all need to be moved into the header? https://codereview.chromium.org/1749073002/diff/60001/base/memory/memory_pres... base/memory/memory_pressure_listener.h:122: static MemoryPressureListenerObservers* g_observers; why does this need to be moved into the header? https://codereview.chromium.org/1749073002/diff/60001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1749073002/diff/60001/content/renderer/render... content/renderer/render_thread_impl.cc:2178: v8::MemoryPressureLevel memoryPressureLevel = v8_memory_pressure_level or something of that nature, content uses hacker_case variables. https://codereview.chromium.org/1749073002/diff/60001/content/renderer/render... content/renderer/render_thread_impl.cc:2182: memoryPressureLevel = v8::MemoryPressureLevel::kNone; Sigh, not having base in v8 makes for really silly code. Why does base/ use -1, 0, 2 instead of regular enum indexing for this? If the enums were parallel we could just static_assert and cast between them. https://codereview.chromium.org/1749073002/diff/60001/content/renderer/render... File content/renderer/render_thread_impl.h (right): https://codereview.chromium.org/1749073002/diff/60001/content/renderer/render... content/renderer/render_thread_impl.h:35: #include "v8/include/v8.h" do this in the .cc file, it doesn't seem used here.
https://codereview.chromium.org/1749073002/diff/60001/base/memory/memory_pres... File base/memory/memory_pressure_listener.h (right): https://codereview.chromium.org/1749073002/diff/60001/base/memory/memory_pres... base/memory/memory_pressure_listener.h:122: static MemoryPressureListenerObservers* g_observers; esprehn wrote: > why does this need to be moved into the header? I said in https://codereview.chromium.org/1749073002/diff/20001/base/memory/memory_pres... that I thought that it was gross that this singleton object kept some of its state in member variables and some of its state in globals. But what was done here is not what I meant at all. This just moved the global to a static class member, which is the same thing (and private static data usually doesn’t need to be a class member at all). So no, this is wrong. I meant that this should stop being static. And when things like this move around, it’s appropriate to fix the name: the g_ is no longer applicable, but a trailing underscore is.
https://codereview.chromium.org/1749073002/diff/60001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1749073002/diff/60001/content/renderer/render... content/renderer/render_thread_impl.cc:2193: blink::mainThreadIsolate()->MemoryPressureNotification(memoryPressureLevel); since this API is threadsafe, why not notify the workers as well?
https://codereview.chromium.org/1749073002/diff/60001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1749073002/diff/60001/content/renderer/render... content/renderer/render_thread_impl.cc:2180: switch (memory_pressure_level) { this should probably also take into account whether the renderer is in the foreground or not?
PTAL https://codereview.chromium.org/1749073002/diff/60001/base/memory/memory_pres... File base/memory/memory_pressure_listener.h (right): https://codereview.chromium.org/1749073002/diff/60001/base/memory/memory_pres... base/memory/memory_pressure_listener.h:50: // ObserverListThreadSafe is RefCountedThreadSafe, this traits is needed On 2016/03/25 09:20:35, esprehn wrote: > Why does this all need to be moved into the header? According to Mark's comments https://codereview.chromium.org/1749073002/diff/60001/base/memory/memory_pres... base/memory/memory_pressure_listener.h:122: static MemoryPressureListenerObservers* g_observers; On 2016/03/25 16:59:31, Mark Mentovai wrote: > esprehn wrote: > > why does this need to be moved into the header? > > I said in > https://codereview.chromium.org/1749073002/diff/20001/base/memory/memory_pres... > that I thought that it was gross that this singleton object kept some of its > state in member variables and some of its state in globals. > > But what was done here is not what I meant at all. This just moved the global to > a static class member, which is the same thing (and private static data usually > doesn’t need to be a class member at all). So no, this is wrong. > > I meant that this should stop being static. And when things like this move > around, it’s appropriate to fix the name: the g_ is no longer applicable, but a > trailing underscore is. I am sorry to misunderstand Mark's comments before. I have updated the CL according to renewed understanding, https://codereview.chromium.org/1749073002/diff/60001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1749073002/diff/60001/content/renderer/render... content/renderer/render_thread_impl.cc:2178: v8::MemoryPressureLevel memoryPressureLevel = On 2016/03/25 09:20:35, esprehn wrote: > v8_memory_pressure_level or something of that nature, content uses hacker_case > variables. Done. https://codereview.chromium.org/1749073002/diff/60001/content/renderer/render... content/renderer/render_thread_impl.cc:2180: switch (memory_pressure_level) { On 2016/03/30 16:23:01, jochen wrote: > this should probably also take into account whether the renderer is in the > foreground or not? The CL can deliver memory pressure notification to V8 directly and V8 only does immediate GC for critical notification. Considering that critical notification means background process will be killed and foreground process also has OOM risk, such as BUG=590975. So maybe we need deliver memory pressure notification to V8 in foreground. https://codereview.chromium.org/1749073002/diff/60001/content/renderer/render... content/renderer/render_thread_impl.cc:2182: memoryPressureLevel = v8::MemoryPressureLevel::kNone; On 2016/03/25 09:20:35, esprehn wrote: > Sigh, not having base in v8 makes for really silly code. Why does base/ use -1, > 0, 2 instead of regular enum indexing for this? If the enums were parallel we > could just static_assert and cast between them. Done. https://codereview.chromium.org/1749073002/diff/60001/content/renderer/render... content/renderer/render_thread_impl.cc:2193: blink::mainThreadIsolate()->MemoryPressureNotification(memoryPressureLevel); On 2016/03/30 16:22:23, jochen wrote: > since this API is threadsafe, why not notify the workers as well? Done. https://codereview.chromium.org/1749073002/diff/60001/content/renderer/render... File content/renderer/render_thread_impl.h (right): https://codereview.chromium.org/1749073002/diff/60001/content/renderer/render... content/renderer/render_thread_impl.h:35: #include "v8/include/v8.h" On 2016/03/25 09:20:35, esprehn wrote: > do this in the .cc file, it doesn't seem used here. Done.
https://codereview.chromium.org/1749073002/diff/60001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1749073002/diff/60001/content/renderer/render... content/renderer/render_thread_impl.cc:2180: switch (memory_pressure_level) { On 2016/04/01 at 09:22:34, hong.zheng wrote: > On 2016/03/30 16:23:01, jochen wrote: > > this should probably also take into account whether the renderer is in the > > foreground or not? > > The CL can deliver memory pressure notification to V8 directly and V8 only does immediate GC for critical notification. Considering that critical notification means background process will be killed and foreground process also has OOM risk, such as BUG=590975. So maybe we need deliver memory pressure notification to V8 in foreground. V8 doesn't look at whether the renderer is in the foreground or not. We should translate a android memory pressure event + (foreground state) to the appropriate v8 memory pressure event here. https://codereview.chromium.org/1749073002/diff/100001/content/renderer/rende... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1749073002/diff/100001/content/renderer/rende... content/renderer/render_thread_impl.cc:2194: RenderThread::Get()->PostTaskToAllWebWorkers( it would be nicer if we'd also synchrounously notified the workers
https://codereview.chromium.org/1749073002/diff/60001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1749073002/diff/60001/content/renderer/render... content/renderer/render_thread_impl.cc:2180: switch (memory_pressure_level) { On 2016/04/03 11:21:35, jochen wrote: > On 2016/04/01 at 09:22:34, hong.zheng wrote: > > On 2016/03/30 16:23:01, jochen wrote: > > > this should probably also take into account whether the renderer is in the > > > foreground or not? > > > > The CL can deliver memory pressure notification to V8 directly and V8 only > does immediate GC for critical notification. Considering that critical > notification means background process will be killed and foreground process also > has OOM risk, such as BUG=590975. So maybe we need deliver memory pressure > notification to V8 in foreground. > > V8 doesn't look at whether the renderer is in the foreground or not. We should > translate a android memory pressure event + (foreground state) to the > appropriate v8 memory pressure event here. It is a problem about how to balance OOM risk and performance. There are two solutions: 1. use the same memory pressure level as chromium in V8 for foreground renderer and translate chromium critical and moderate level to V8 critical level for background renderer. 2. translate chromium critical and moderate level to V8 moderate level for foreground renderer and use the same memory pressure level as chromium in V8 for background renderer which one do you think more make sense?
I think 2) makes sense
https://codereview.chromium.org/1749073002/diff/60001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1749073002/diff/60001/content/renderer/render... content/renderer/render_thread_impl.cc:2180: switch (memory_pressure_level) { On 2016/04/03 11:21:35, jochen - slow wrote: > On 2016/04/01 at 09:22:34, hong.zheng wrote: > > On 2016/03/30 16:23:01, jochen wrote: > > > this should probably also take into account whether the renderer is in the > > > foreground or not? > > > > The CL can deliver memory pressure notification to V8 directly and V8 only > does immediate GC for critical notification. Considering that critical > notification means background process will be killed and foreground process also > has OOM risk, such as BUG=590975. So maybe we need deliver memory pressure > notification to V8 in foreground. > > V8 doesn't look at whether the renderer is in the foreground or not. We should > translate a android memory pressure event + (foreground state) to the > appropriate v8 memory pressure event here. Done. https://codereview.chromium.org/1749073002/diff/100001/content/renderer/rende... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1749073002/diff/100001/content/renderer/rende... content/renderer/render_thread_impl.cc:2194: RenderThread::Get()->PostTaskToAllWebWorkers( On 2016/04/03 11:21:36, jochen - slow wrote: > it would be nicer if we'd also synchrounously notified the workers Done.
jochen@chromium.org changed reviewers: + kinuko@chromium.org
+kinuko Elliott/Kinuko, could you comment on the API to get the v8::Isolate* for all workers?
https://codereview.chromium.org/1749073002/diff/120001/base/memory/memory_pre... File base/memory/memory_pressure_listener.h (right): https://codereview.chromium.org/1749073002/diff/120001/base/memory/memory_pre... base/memory/memory_pressure_listener.h:73: explicit MemoryPressureListener( nit: no need of explicit here https://codereview.chromium.org/1749073002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1749073002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:217: gIsolates->insert(m_isolate); For compositor worker (where single thread is shared by multiple workers) initializeIsolate() returns the same isolate for multiple times, and destroyIsolate() may not be actually destroying the isolate yet if there're other compositor workers running on the same thread. We're working on making the part slightly cleaner but for the time being.. maybe destroyIsolate() could return a bool value that indicates if it actually destroyed the isolate or not? https://codereview.chromium.org/1749073002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:513: return gIsolates; We seem to be accessing this set from multiple threads without locking? https://codereview.chromium.org/1749073002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.h (right): https://codereview.chromium.org/1749073002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.h:118: static std::set<v8::Isolate*>* workerThreadIsolates(); Why we return mutable pointer to the set here? https://codereview.chromium.org/1749073002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.h:181: static std::set<v8::Isolate*>* gIsolates; Why do we need to have this in the header?
thanks for your comments. I have modified the CL, PTAL. https://codereview.chromium.org/1749073002/diff/120001/base/memory/memory_pre... File base/memory/memory_pressure_listener.h (right): https://codereview.chromium.org/1749073002/diff/120001/base/memory/memory_pre... base/memory/memory_pressure_listener.h:73: explicit MemoryPressureListener( On 2016/04/11 09:30:40, kinuko wrote: > nit: no need of explicit here Done. https://codereview.chromium.org/1749073002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1749073002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:217: gIsolates->insert(m_isolate); On 2016/04/11 09:30:40, kinuko wrote: > For compositor worker (where single thread is shared by multiple workers) > initializeIsolate() returns the same isolate for multiple times, and > destroyIsolate() may not be actually destroying the isolate yet if there're > other compositor workers running on the same thread. > > We're working on making the part slightly cleaner but for the time being.. maybe > destroyIsolate() could return a bool value that indicates if it actually > destroyed the isolate or not? use WorkerBackingThread instead https://codereview.chromium.org/1749073002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:513: return gIsolates; On 2016/04/11 09:30:40, kinuko wrote: > We seem to be accessing this set from multiple threads without locking? Done. https://codereview.chromium.org/1749073002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.h (right): https://codereview.chromium.org/1749073002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.h:118: static std::set<v8::Isolate*>* workerThreadIsolates(); On 2016/04/11 09:30:40, kinuko wrote: > Why we return mutable pointer to the set here? Done. https://codereview.chromium.org/1749073002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.h:181: static std::set<v8::Isolate*>* gIsolates; On 2016/04/11 09:30:40, kinuko wrote: > Why do we need to have this in the header? Done.
Thanks, looks better now https://codereview.chromium.org/1749073002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp (right): https://codereview.chromium.org/1749073002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:29: DEFINE_STATIC_LOCAL(std::set<v8::Isolate*>, isolates, ()); This might fail thread assertion check (that's a part of DEFINE_STATIC_LOCAL) as the first call to this static might happen on any worker thread and this gets accessed by multiple thread-- you may want to follow the way as what guidToVersionMap() does in Database.cpp does. https://codereview.chromium.org/1749073002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:82: } nit: could we extract these logic out as static function (or a function in anon-namespace) so that we don't need to have nested blocks for scoped locks? addWorkerIsolate(m_isolate) removeWorkerIsolate(m_isolate) https://codereview.chromium.org/1749073002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:124: isolate->MemoryPressureNotification(level); Is this method safe to call within a lock / doesn't take long time?
some more comments https://codereview.chromium.org/1749073002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp (right): https://codereview.chromium.org/1749073002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:17: #include <set> include order looks wrong Could we use HashSet in blink instead?
https://codereview.chromium.org/1749073002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp (right): https://codereview.chromium.org/1749073002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:17: #include <set> On 2016/04/18 09:14:11, kinuko wrote: > include order looks wrong > > Could we use HashSet in blink instead? Done. https://codereview.chromium.org/1749073002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:29: DEFINE_STATIC_LOCAL(std::set<v8::Isolate*>, isolates, ()); On 2016/04/18 08:56:19, kinuko wrote: > This might fail thread assertion check (that's a part of DEFINE_STATIC_LOCAL) as > the first call to this static might happen on any worker thread and this gets > accessed by multiple thread-- you may want to follow the way as what > guidToVersionMap() does in Database.cpp does. Done. https://codereview.chromium.org/1749073002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:82: } On 2016/04/18 08:56:19, kinuko wrote: > nit: could we extract these logic out as static function (or a function in > anon-namespace) so that we don't need to have nested blocks for scoped locks? > > addWorkerIsolate(m_isolate) > removeWorkerIsolate(m_isolate) Done. https://codereview.chromium.org/1749073002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:124: isolate->MemoryPressureNotification(level); On 2016/04/18 08:56:19, kinuko wrote: > Is this method safe to call within a lock / doesn't take long time? MemoryPressureNotification normally sends memory pressure notification level to V8 and requests a interrupt to do GC. It is thread safe and won't take long time.
https://codereview.chromium.org/1749073002/diff/160001/content/renderer/rende... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1749073002/diff/160001/content/renderer/rende... content/renderer/render_thread_impl.cc:2139: if (blink::mainThreadIsolate()) { early return instead of indenting the entire function https://codereview.chromium.org/1749073002/diff/160001/content/renderer/rende... content/renderer/render_thread_impl.cc:2142: base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_NONE) == these static_asserts should just be in a block at the top of the file, they don't need to be inside the function https://codereview.chromium.org/1749073002/diff/160001/content/renderer/rende... content/renderer/render_thread_impl.cc:2157: if (v8_memory_pressure_level == v8::MemoryPressureLevel::kCritical) combine the if statements, doing: if (x) { if (y) // comment thing(); } is very hard to read. // comment if (x && y) thing(); https://codereview.chromium.org/1749073002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp (right): https://codereview.chromium.org/1749073002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:130: for (HashSet<v8::Isolate*>::iterator it = isolates().begin(); range loop for (v8::Isolate* isolate : isolates()) isolate-> MemoryPressureNotification(level)
Thanks for comments https://codereview.chromium.org/1749073002/diff/160001/content/renderer/rende... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1749073002/diff/160001/content/renderer/rende... content/renderer/render_thread_impl.cc:2139: if (blink::mainThreadIsolate()) { On 2016/04/18 21:57:59, esprehn wrote: > early return instead of indenting the entire function Done. https://codereview.chromium.org/1749073002/diff/160001/content/renderer/rende... content/renderer/render_thread_impl.cc:2142: base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_NONE) == On 2016/04/18 21:57:59, esprehn wrote: > these static_asserts should just be in a block at the top of the file, they > don't need to be inside the function Done. https://codereview.chromium.org/1749073002/diff/160001/content/renderer/rende... content/renderer/render_thread_impl.cc:2157: if (v8_memory_pressure_level == v8::MemoryPressureLevel::kCritical) On 2016/04/18 21:57:58, esprehn wrote: > combine the if statements, doing: > > if (x) { > if (y) > // comment > thing(); > } > > is very hard to read. > > // comment > if (x && y) > thing(); Done. https://codereview.chromium.org/1749073002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp (right): https://codereview.chromium.org/1749073002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:130: for (HashSet<v8::Isolate*>::iterator it = isolates().begin(); On 2016/04/18 21:57:59, esprehn wrote: > range loop > > for (v8::Isolate* isolate : isolates()) > isolate-> MemoryPressureNotification(level) Done.
lgtm for worker / the part I care about
lgtm with comments https://codereview.chromium.org/1749073002/diff/180001/content/renderer/rende... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1749073002/diff/180001/content/renderer/rende... content/renderer/render_thread_impl.cc:2157: // moderate level for foregroud renderer. foregrou*n*d https://codereview.chromium.org/1749073002/diff/180001/content/renderer/rende... content/renderer/render_thread_impl.cc:2159: v8_memory_pressure_level == v8::MemoryPressureLevel::kCritical) please add { }
https://codereview.chromium.org/1749073002/diff/180001/base/memory/memory_pre... File base/memory/memory_pressure_listener.cc (right): https://codereview.chromium.org/1749073002/diff/180001/base/memory/memory_pre... base/memory/memory_pressure_listener.cc:19: new MemoryPressureListener(); This results in a module initializer. Can’t do that.
thanks for comments. PTAL https://codereview.chromium.org/1749073002/diff/180001/base/memory/memory_pre... File base/memory/memory_pressure_listener.cc (right): https://codereview.chromium.org/1749073002/diff/180001/base/memory/memory_pre... base/memory/memory_pressure_listener.cc:19: new MemoryPressureListener(); On 2016/04/20 13:21:33, Mark Mentovai wrote: > This results in a module initializer. Can’t do that. Done. https://codereview.chromium.org/1749073002/diff/180001/content/renderer/rende... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1749073002/diff/180001/content/renderer/rende... content/renderer/render_thread_impl.cc:2157: // moderate level for foregroud renderer. On 2016/04/20 12:46:35, jochen wrote: > foregrou*n*d Done. https://codereview.chromium.org/1749073002/diff/180001/content/renderer/rende... content/renderer/render_thread_impl.cc:2159: v8_memory_pressure_level == v8::MemoryPressureLevel::kCritical) On 2016/04/20 12:46:35, jochen wrote: > please add { } Done.
https://codereview.chromium.org/1749073002/diff/200001/base/memory/memory_pre... File base/memory/memory_pressure_listener.cc (right): https://codereview.chromium.org/1749073002/diff/200001/base/memory/memory_pre... base/memory/memory_pressure_listener.cc:35: base::AutoLock lock(GlobalListener()->sync_observers_lock_.Get()); Same here and on lines 59 and 111. https://codereview.chromium.org/1749073002/diff/200001/base/memory/memory_pre... base/memory/memory_pressure_listener.cc:42: if (!GlobalListener()) Why would there not be any GlobalListener()? You’re guarding it here and on line 51, but not 34 or 58. https://codereview.chromium.org/1749073002/diff/200001/base/memory/memory_pre... base/memory/memory_pressure_listener.cc:119: static MemoryPressureListener* listener = new MemoryPressureListener(); This is the thing that should have been the lazy instance. The member variables don’t need to be. This being lazy would protect those. https://codereview.chromium.org/1749073002/diff/200001/base/memory/memory_pre... base/memory/memory_pressure_listener.cc:120: return listener; Which functions get called on the global listener? Which functions get called on other listeners? It’s hard to tell, because everything’s all in one class. Whose member variables are relevant in any given instantiation? Nobody knows! It seems like those two concepts should be split into distinct classes. https://codereview.chromium.org/1749073002/diff/200001/base/memory/memory_pre... File base/memory/memory_pressure_listener.h (right): https://codereview.chromium.org/1749073002/diff/200001/base/memory/memory_pre... base/memory/memory_pressure_listener.h:127: base::LazyInstance<base::Lock>::Leaky sync_observers_lock_; You don’t need base:: qualification in namespace base. Same on lines 68-69, 94, and 99.
PTAL https://codereview.chromium.org/1749073002/diff/200001/base/memory/memory_pre... File base/memory/memory_pressure_listener.cc (right): https://codereview.chromium.org/1749073002/diff/200001/base/memory/memory_pre... base/memory/memory_pressure_listener.cc:119: static MemoryPressureListener* listener = new MemoryPressureListener(); On 2016/04/21 20:45:33, Mark Mentovai wrote: > This is the thing that should have been the lazy instance. The member variables > don’t need to be. This being lazy would protect those. Done. https://codereview.chromium.org/1749073002/diff/200001/base/memory/memory_pre... base/memory/memory_pressure_listener.cc:120: return listener; On 2016/04/21 20:45:33, Mark Mentovai wrote: > Which functions get called on the global listener? Which functions get called on > other listeners? It’s hard to tell, because everything’s all in one class. Whose > member variables are relevant in any given instantiation? Nobody knows! It seems > like those two concepts should be split into distinct classes. Done. https://codereview.chromium.org/1749073002/diff/200001/base/memory/memory_pre... File base/memory/memory_pressure_listener.h (right): https://codereview.chromium.org/1749073002/diff/200001/base/memory/memory_pre... base/memory/memory_pressure_listener.h:127: base::LazyInstance<base::Lock>::Leaky sync_observers_lock_; On 2016/04/21 20:45:33, Mark Mentovai wrote: > You don’t need base:: qualification in namespace base. Same on lines 68-69, 94, > and 99. Done.
LGTM in base now.
@esprehn, PTAL
lgtm
The CQ bit was checked by hong.zheng@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, jochen@chromium.org Link to the patchset: https://codereview.chromium.org/1749073002/#ps220001 (title: "extract MemoryPressureObserver from MemoryPressureListener")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749073002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749073002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
The CQ bit was checked by hong.zheng@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749073002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749073002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by hong.zheng@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, jochen@chromium.org, esprehn@chromium.org, mark@chromium.org Link to the patchset: https://codereview.chromium.org/1749073002/#ps240001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749073002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749073002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by hong.zheng@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749073002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749073002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by hong.zheng@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749073002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749073002/240001
Message was sent while issue was closed.
Description was changed from ========== Do V8 GC ASAP if system memory is pressured Currently after receiving memory pressure notification, Chrome posts asynchronous task to renderer main thread to trigger V8 GC. If renderer main thread is busy, purging memory task will be postponed running, which increases OOM risk. The CL can send memory pressure notification to renderer main thread synchronously and trigger V8 GC while V8 is in the middle of executing JavaScript. The CL depends on CL 1813963002 BUG=590975 ========== to ========== Do V8 GC ASAP if system memory is pressured Currently after receiving memory pressure notification, Chrome posts asynchronous task to renderer main thread to trigger V8 GC. If renderer main thread is busy, purging memory task will be postponed running, which increases OOM risk. The CL can send memory pressure notification to renderer main thread synchronously and trigger V8 GC while V8 is in the middle of executing JavaScript. The CL depends on CL 1813963002 BUG=590975 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Do V8 GC ASAP if system memory is pressured Currently after receiving memory pressure notification, Chrome posts asynchronous task to renderer main thread to trigger V8 GC. If renderer main thread is busy, purging memory task will be postponed running, which increases OOM risk. The CL can send memory pressure notification to renderer main thread synchronously and trigger V8 GC while V8 is in the middle of executing JavaScript. The CL depends on CL 1813963002 BUG=590975 ========== to ========== Do V8 GC ASAP if system memory is pressured Currently after receiving memory pressure notification, Chrome posts asynchronous task to renderer main thread to trigger V8 GC. If renderer main thread is busy, purging memory task will be postponed running, which increases OOM risk. The CL can send memory pressure notification to renderer main thread synchronously and trigger V8 GC while V8 is in the middle of executing JavaScript. The CL depends on CL 1813963002 BUG=590975 Committed: https://crrev.com/f80422e52658fd87d4159bfeeddd703bae1d4180 Cr-Commit-Position: refs/heads/master@{#391167} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/f80422e52658fd87d4159bfeeddd703bae1d4180 Cr-Commit-Position: refs/heads/master@{#391167}
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:240001) has been created in https://codereview.chromium.org/1953483002/ by vitalybuka@chromium.org. The reason for reverting is: Crash in CallOnForegroundThread BUG=609056.
Message was sent while issue was closed.
https://codereview.chromium.org/1749073002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp (right): https://codereview.chromium.org/1749073002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:121: removeWorkerIsolate(m_isolate); i suspect this needs to come before V8PerIsolateData::destroy
Message was sent while issue was closed.
On 2016/05/04 18:28:14, jochen wrote: > https://codereview.chromium.org/1749073002/diff/240001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp (right): > > https://codereview.chromium.org/1749073002/diff/240001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:121: > removeWorkerIsolate(m_isolate); > i suspect this needs to come before V8PerIsolateData::destroy Thanks jochen's guidance. I verified removing worker thread isolate after V8PerIsolateData::destroy could cause crash. So I fix it and upload the CL again in https://codereview.chromium.org/1949153003 |
