Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
413113003:
Oilpan: Prepare moving AsyncCallChain to Oilpan (Closed)
|
Created:
6 years, 5 months ago by keishi Modified:
6 years, 4 months ago CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, malch+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, sergeyv+blink_chromium.org, aandrey+blink_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
DescriptionOilpan: Prepare moving AsyncCallChain to Oilpan
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179931
Patch Set 1 #Patch Set 2 : #
Total comments: 9
Patch Set 3 : Changed to memeber #Patch Set 4 : #
Total comments: 7
Patch Set 5 : #
Total comments: 1
Patch Set 6 : Rebased #
Messages
Total messages: 21 (0 generated)
https://codereview.chromium.org/413113003/diff/20001/Source/core/inspector/As... File Source/core/inspector/AsyncCallStackTracker.cpp (right): https://codereview.chromium.org/413113003/diff/20001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:94: WillBePersistentHeapHashMap<Event*, RefPtrWillBeMember<AsyncCallChain> > m_eventCallChains; Event* should be WeakMember<Event> or Member<Event>.
https://codereview.chromium.org/413113003/diff/20001/Source/core/inspector/As... File Source/core/inspector/AsyncCallStackTracker.cpp (right): https://codereview.chromium.org/413113003/diff/20001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:59: class AsyncCallStackTracker::ExecutionContextData FINAL : public ContextLifecycleObserver { Consider moving ExecutionContextData in a follow-up. ExecutionContextData has a lot of persistent handles. https://codereview.chromium.org/413113003/diff/20001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:95: WillBePersistentHeapHashMap<EventTarget*, RefPtrWillBeMember<AsyncCallChain> > m_xhrCallChains; EventTarget is on-heap as well. https://codereview.chromium.org/413113003/diff/20001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:96: WillBePersistentHeapHashMap<MutationObserver*, RefPtrWillBeMember<AsyncCallChain> > m_mutationObserverCallChains; MutationObserver is on-heap as well. https://codereview.chromium.org/413113003/diff/20001/Source/core/inspector/As... File Source/core/inspector/AsyncCallStackTracker.h (right): https://codereview.chromium.org/413113003/diff/20001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.h:52: class AsyncCallStackTracker { Consider moving AsyncCallStackTracker to the heap in a follow-up.
Also consider adding FINAL if possible to classes with a non-virtual trace method.
> Also consider adding FINAL if possible to classes with a non-virtual trace method. Done. https://codereview.chromium.org/413113003/diff/20001/Source/core/inspector/As... File Source/core/inspector/AsyncCallStackTracker.cpp (right): https://codereview.chromium.org/413113003/diff/20001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:94: WillBePersistentHeapHashMap<Event*, RefPtrWillBeMember<AsyncCallChain> > m_eventCallChains; On 2014/07/29 02:33:54, tkent wrote: > Event* should be WeakMember<Event> or Member<Event>. Done. https://codereview.chromium.org/413113003/diff/20001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:95: WillBePersistentHeapHashMap<EventTarget*, RefPtrWillBeMember<AsyncCallChain> > m_xhrCallChains; On 2014/07/29 03:43:39, haraken wrote: > > EventTarget is on-heap as well. Done. https://codereview.chromium.org/413113003/diff/20001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:96: WillBePersistentHeapHashMap<MutationObserver*, RefPtrWillBeMember<AsyncCallChain> > m_mutationObserverCallChains; On 2014/07/29 03:43:39, haraken wrote: > > MutationObserver is on-heap as well. Done. https://codereview.chromium.org/413113003/diff/20001/Source/core/inspector/As... File Source/core/inspector/AsyncCallStackTracker.h (right): https://codereview.chromium.org/413113003/diff/20001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.h:52: class AsyncCallStackTracker { On 2014/07/29 03:43:39, haraken wrote: > > Consider moving AsyncCallStackTracker to the heap in a follow-up. I'll do it in a follow-up
https://codereview.chromium.org/413113003/diff/60001/Source/core/inspector/Ne... File Source/core/inspector/NetworkResourcesData.h (right): https://codereview.chromium.org/413113003/diff/60001/Source/core/inspector/Ne... Source/core/inspector/NetworkResourcesData.h:53: : public RefCountedWillBeGarbageCollectedFinalized<XHRReplayData> Is this related to this CL?
+aandrey
looks good https://codereview.chromium.org/413113003/diff/60001/Source/core/inspector/As... File Source/core/inspector/AsyncCallStackTracker.cpp (right): https://codereview.chromium.org/413113003/diff/60001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:92: WillBePersistentHeapHashMap<int, RefPtrWillBeMember<AsyncCallChain> > m_timerCallChains; shouldn't these hash maps be visited somewhere in a trace() method? https://codereview.chromium.org/413113003/diff/60001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:97: WillBePersistentHeapHashMap<ExecutionContextTask*, RefPtrWillBeMember<AsyncCallChain> > m_executionContextTaskCallChains; are you planning moving ExecutionContextTask to the heap as well?
https://codereview.chromium.org/413113003/diff/60001/Source/core/inspector/As... File Source/core/inspector/AsyncCallStackTracker.cpp (right): https://codereview.chromium.org/413113003/diff/60001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:92: WillBePersistentHeapHashMap<int, RefPtrWillBeMember<AsyncCallChain> > m_timerCallChains; On 2014/07/29 09:09:41, aandrey wrote: > shouldn't these hash maps be visited somewhere in a trace() method? AsyncCallStackTracker::ExecutionContextData is not on the heap yet so I used _Persistent_HeapHashMap which doesn't need manual visiting. https://codereview.chromium.org/413113003/diff/60001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:97: WillBePersistentHeapHashMap<ExecutionContextTask*, RefPtrWillBeMember<AsyncCallChain> > m_executionContextTaskCallChains; On 2014/07/29 09:09:41, aandrey wrote: > are you planning moving ExecutionContextTask to the heap as well? I'm not moving ExecutionContextTask to the heap for now. https://codereview.chromium.org/413113003/diff/60001/Source/core/inspector/Ne... File Source/core/inspector/NetworkResourcesData.h (right): https://codereview.chromium.org/413113003/diff/60001/Source/core/inspector/Ne... Source/core/inspector/NetworkResourcesData.h:53: : public RefCountedWillBeGarbageCollectedFinalized<XHRReplayData> On 2014/07/29 08:04:06, tkent wrote: > Is this related to this CL? Sorry, other CLs got mixed in.
lgtm
LGTM I'm not confident that persistent handles introduced in this CL doesn't cause cycles, but I'm fine as long as you get rid of those persistent handles in a follow-up CL. https://codereview.chromium.org/413113003/diff/60001/Source/core/inspector/As... File Source/core/inspector/AsyncCallStackTracker.cpp (right): https://codereview.chromium.org/413113003/diff/60001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:97: WillBePersistentHeapHashMap<ExecutionContextTask*, RefPtrWillBeMember<AsyncCallChain> > m_executionContextTaskCallChains; On 2014/07/29 09:32:37, keishi wrote: > On 2014/07/29 09:09:41, aandrey wrote: > > are you planning moving ExecutionContextTask to the heap as well? > > I'm not moving ExecutionContextTask to the heap for now. FYI, we cannot move ExecutionContextTask to the heap easily because an ExeutionContextTask object can outlive a thread that created the object. Oilpan doesn't support such objects. https://codereview.chromium.org/413113003/diff/80001/Source/core/inspector/As... File Source/core/inspector/AsyncCallStackTracker.h (right): https://codereview.chromium.org/413113003/diff/80001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.h:135: typedef HashMap<ExecutionContext*, ExecutionContextData*> ExecutionContextDataMap; Not related to this CL, ExecutionContextData* should be an OwnPtr. You can make the change when moving ExecutionContextData to the heap in a follow-up CL.
> Not related to this CL, ExecutionContextData* should be an OwnPtr. You can make > the change when moving ExecutionContextData to the heap in a follow-up CL. That's true.
lgtm
The CQ bit was checked by keishi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keishi@chromium.org/413113003/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/19230)
Message was sent while issue was closed.
Change committed as 179931
Message was sent while issue was closed.
Is there a followup CL imminent to address the GC plugin complaining that InspectorDebuggerAgent (a GCed object) now having a field containing a Persistent? http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...
Message was sent while issue was closed.
On 2014/08/11 10:20:24, sof wrote: > Is there a followup CL imminent to address the GC plugin complaining that > InspectorDebuggerAgent (a GCed object) now having a field containing a > Persistent? > > http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%... Sorry. Reverting right now.
Message was sent while issue was closed.
On 2014/08/11 10:23:38, keishi wrote: > On 2014/08/11 10:20:24, sof wrote: > > Is there a followup CL imminent to address the GC plugin complaining that > > InspectorDebuggerAgent (a GCed object) now having a field containing a > > Persistent? > > > > > http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%... > > Sorry. Reverting right now. I will merge this CL to https://codereview.chromium.org/422273002/
Message was sent while issue was closed.
On 2014/08/11 10:38:24, keishi wrote: > On 2014/08/11 10:23:38, keishi wrote: > > On 2014/08/11 10:20:24, sof wrote: > > > Is there a followup CL imminent to address the GC plugin complaining that > > > InspectorDebuggerAgent (a GCed object) now having a field containing a > > > Persistent? > > > > > > > > > http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%... > > > > Sorry. Reverting right now. > > I will merge this CL to https://codereview.chromium.org/422273002/ Thanks very much for doing this & so quickly! (landing a temporary GC_PLUGIN_IGNORE() for the problematic field would have been an alternative.) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
