Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
131803005:
Add more oilpan collections support (Closed)
|
Created:
6 years, 11 months ago by Erik Corry Modified:
5 years, 11 months ago CC:
blink-reviews, loislo+blink_chromium.org, yurys+blink_chromium.org, abarth-chromium, adamk+blink_chromium.org, Inactive Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionAdd more oilpan collections support
R=ager@chromium.org, cevans@chromium.org, haraken@chromium.org, wibling@chromium.org, zerny@chromium.org
BUG=
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165932
Patch Set 1 #
Total comments: 29
Patch Set 2 : Code review feedback #Patch Set 3 : Merge up #Patch Set 4 : Fix raw -> get for realz #Patch Set 5 : Fix compile error in gcc. #Patch Set 6 : Add missing getOther method on DefaultAllocatort #Patch Set 7 : MSVC fix again #Patch Set 8 : Merged up and tests fixed. #Patch Set 9 : Fix OtherType for Windows compielr #
Total comments: 2
Messages
Total messages: 30 (1 generated)
LGTM with comments. https://codereview.chromium.org/131803005/diff/1/Source/heap/Handle.h File Source/heap/Handle.h (right): https://codereview.chromium.org/131803005/diff/1/Source/heap/Handle.h#newcode350 Source/heap/Handle.h:350: protected: Don't we want private here? https://codereview.chromium.org/131803005/diff/1/Source/heap/Handle.h#newcode484 Source/heap/Handle.h:484: OffHeapCollectionTraceTrait<Collection>::mark(visitor, m_collection); } Write } in the next line. https://codereview.chromium.org/131803005/diff/1/Source/heap/Heap.h File Source/heap/Heap.h (right): https://codereview.chromium.org/131803005/diff/1/Source/heap/Heap.h#newcode1204 Source/heap/Heap.h:1204: static void markUsingGCInfo(Visitor* visitor, const void* buffer) How about just calling this method "mark"? Or probably "traceUsingGCInfo"? (I don't fully understand whether this method should be called "mark" or "trace".) https://codereview.chromium.org/131803005/diff/1/Source/heap/Heap.h#newcode1209 Source/heap/Heap.h:1209: static void markNoTracing(Visitor* visitor, const void* t) markNoTracing => markWithoutTracing ? https://codereview.chromium.org/131803005/diff/1/Source/heap/Heap.h#newcode1390 Source/heap/Heap.h:1390: inline void clearWeakMap(Visitor* visitor, MapType& map, void (*callback)(T, const typename MapType::KeyType&, typename MapType::MappedType), T data) Can we make |callback| and |data| default parameters, and remove 'inline void clearWeakMap(Visitor* visitor, MapType& map)'? https://codereview.chromium.org/131803005/diff/1/Source/heap/Heap.h#newcode1408 Source/heap/Heap.h:1408: inline void clearWeakMapValues(Visitor* visitor, MapType& map) When is this method used? It looks a bit strange to me that we want to remove keys when corresponding values are not alive. https://codereview.chromium.org/131803005/diff/1/Source/heap/HeapTest.cpp File Source/heap/HeapTest.cpp (right): https://codereview.chromium.org/131803005/diff/1/Source/heap/HeapTest.cpp#new... Source/heap/HeapTest.cpp:950: Heap::init(); This was removed by wibling's CL. https://codereview.chromium.org/131803005/diff/1/Source/heap/HeapTest.cpp#new... Source/heap/HeapTest.cpp:1021: Heap::init(); Ditto. https://codereview.chromium.org/131803005/diff/1/Source/heap/HeapTest.cpp#new... Source/heap/HeapTest.cpp:1211: Heap::init(); Ditto. https://codereview.chromium.org/131803005/diff/1/Source/heap/HeapTest.cpp#new... Source/heap/HeapTest.cpp:1265: Heap::init(); Ditto. https://codereview.chromium.org/131803005/diff/1/Source/heap/Visitor.h File Source/heap/Visitor.h (right): https://codereview.chromium.org/131803005/diff/1/Source/heap/Visitor.h#newcod... Source/heap/Visitor.h:200: static void mark(Visitor*, const Collection&); Shouldn't this be "trace" instead of "mark"? https://codereview.chromium.org/131803005/diff/1/Source/heap/Visitor.h#newcod... Source/heap/Visitor.h:347: static void mark(Visitor* v, const HashSet& set) v => visitor https://codereview.chromium.org/131803005/diff/1/Source/heap/Visitor.h#newcod... Source/heap/Visitor.h:376: static void mark(Visitor* v, const HashMap& map) v => visitor https://codereview.chromium.org/131803005/diff/1/Source/heap/Visitor.h#newcod... Source/heap/Visitor.h:385: } Shouldn't this be: for (typename HashMap::const_iterator it = map.begin(), end = map.end(); it != end; ++it) { if (WTF::NeedsTracing<Key>::value) { CollectionBackingTraceTrait<KeyTraits::needsTracing, KeyTraits::isWeak, false, Key, KeyTraits>::mark(v, it->key); } if (WTF::NeedsTracing<Value>::value) { CollectionBackingTraceTrait<ValueTraits::needsTracing, ValueTraits::isWeak, false, Value, ValueTraits>::mark(v, it->value); } } https://codereview.chromium.org/131803005/diff/1/Source/heap/Visitor.h#newcod... Source/heap/Visitor.h:438: static void mark(Visitor* v, const Vector& vector) v => visitor
lgtm https://codereview.chromium.org/131803005/diff/1/Source/heap/Handle.h File Source/heap/Handle.h (right): https://codereview.chromium.org/131803005/diff/1/Source/heap/Handle.h#newcode350 Source/heap/Handle.h:350: protected: On 2014/01/15 04:48:35, haraken wrote: > > Don't we want private here? WeakMember needs access to m_raw. btw, most of the friends decls will go with Mads's latest CL.
On 2014/01/15 08:05:27, zerny-chromium wrote: > lgtm > > https://codereview.chromium.org/131803005/diff/1/Source/heap/Handle.h > File Source/heap/Handle.h (right): > > https://codereview.chromium.org/131803005/diff/1/Source/heap/Handle.h#newcode350 > Source/heap/Handle.h:350: protected: > On 2014/01/15 04:48:35, haraken wrote: > > > > Don't we want private here? > > WeakMember needs access to m_raw. > > btw, most of the friends decls will go with Mads's latest CL. LGTM
LGTM I have removed a bunch of friend declarations and renamed raw to get in my last changelist. https://codereview.chromium.org/131803005/diff/1/Source/heap/Handle.h File Source/heap/Handle.h (right): https://codereview.chromium.org/131803005/diff/1/Source/heap/Handle.h#newcode47 Source/heap/Handle.h:47: #ifndef NDEBUG Thanks for adding verification; that should make finding bugs with corrupted persistent chains easier! :) We should be able to do that without the #ifndef NDEBUG code by just using the TraceCallback member. Set it to 0 on destruction and check that it is non-zero? You can add a protected method called isAlive to do that check to make it clear what it means. https://codereview.chromium.org/131803005/diff/1/Source/heap/Handle.h#newcode350 Source/heap/Handle.h:350: protected: On 2014/01/15 04:48:35, haraken wrote: > > Don't we want private here? Drive-by answer. :-) We don't need private here. Since we are using conservative scanning there is no longer anything dangerous is getting hold of the raw pointer. Leaving it private makes it simpler to write the comparison operators as well. I have a patch out to rename this to get(), make it public and use it in the comparisons. https://codereview.chromium.org/131803005/diff/1/Source/heap/Handle.h#newcode361 Source/heap/Handle.h:361: template<typename U, typename V> friend bool operator==(const Member<U>&, const Persistent<V>&); Let's git rid of all of these operator friend declarations here. I'll do that in my change to rename raw() to get() and fix comparison operators. https://codereview.chromium.org/131803005/diff/1/Source/heap/Handle.h#newcode444 Source/heap/Handle.h:444: template<typename T, typename U> inline bool operator==(const Member<T>& a, const Persistent<U>& b) { return a.m_raw == b.raw(); } This is a complete mess. Let us just use a.get() == b.get() consistently for all of these. I'll do that with my patch for the ones that are already in the system.
https://codereview.chromium.org/131803005/diff/1/Source/heap/Handle.h File Source/heap/Handle.h (right): https://codereview.chromium.org/131803005/diff/1/Source/heap/Handle.h#newcode484 Source/heap/Handle.h:484: OffHeapCollectionTraceTrait<Collection>::mark(visitor, m_collection); } On 2014/01/15 04:48:35, haraken wrote: > > Write } in the next line. Done. https://codereview.chromium.org/131803005/diff/1/Source/heap/Heap.h File Source/heap/Heap.h (right): https://codereview.chromium.org/131803005/diff/1/Source/heap/Heap.h#newcode1209 Source/heap/Heap.h:1209: static void markNoTracing(Visitor* visitor, const void* t) On 2014/01/15 04:48:35, haraken wrote: > > markNoTracing => markWithoutTracing ? I think we have a little too much naming churn. You approved the old name last week, let's just leave it as it is. https://codereview.chromium.org/131803005/diff/1/Source/heap/Heap.h#newcode1390 Source/heap/Heap.h:1390: inline void clearWeakMap(Visitor* visitor, MapType& map, void (*callback)(T, const typename MapType::KeyType&, typename MapType::MappedType), T data) On 2014/01/15 04:48:35, haraken wrote: > > Can we make |callback| and |data| default parameters, and remove 'inline void > clearWeakMap(Visitor* visitor, MapType& map)'? Actually these routines are there for handling off-heap weak collections. They are a bit dangerous because if you have a GC while you are iterating over them, the current position of the iterator may be invalidated. Therefore we decided to omit support for off-heap weak collections and see if we can just use on-heap weak collections, which are safe for GC during iteration. So I removed this code for now. https://codereview.chromium.org/131803005/diff/1/Source/heap/Heap.h#newcode1408 Source/heap/Heap.h:1408: inline void clearWeakMapValues(Visitor* visitor, MapType& map) On 2014/01/15 04:48:35, haraken wrote: > > When is this method used? It looks a bit strange to me that we want to remove > keys when corresponding values are not alive. This code is gone now, but we have the same semantics for on-heap weak maps. If you have a HeapHashMap<Foo, WeakMember<Bar> > Then if the bar is collected, the entire map entry is removed, rather than just leaving a mapping from a foo to null. I think this is actually the best semantics, since it allows you to GC the foo (in the next GC) and there's rarely any benefit to having a mapping to something that is gone. https://codereview.chromium.org/131803005/diff/1/Source/heap/Visitor.h File Source/heap/Visitor.h (right): https://codereview.chromium.org/131803005/diff/1/Source/heap/Visitor.h#newcod... Source/heap/Visitor.h:200: static void mark(Visitor*, const Collection&); On 2014/01/15 04:48:35, haraken wrote: > > Shouldn't this be "trace" instead of "mark"? Done. https://codereview.chromium.org/131803005/diff/1/Source/heap/Visitor.h#newcod... Source/heap/Visitor.h:347: static void mark(Visitor* v, const HashSet& set) On 2014/01/15 04:48:35, haraken wrote: > > v => visitor Done. https://codereview.chromium.org/131803005/diff/1/Source/heap/Visitor.h#newcod... Source/heap/Visitor.h:376: static void mark(Visitor* v, const HashMap& map) On 2014/01/15 04:48:35, haraken wrote: > > v => visitor Done. https://codereview.chromium.org/131803005/diff/1/Source/heap/Visitor.h#newcod... Source/heap/Visitor.h:385: } On 2014/01/15 04:48:35, haraken wrote: > > Shouldn't this be: > > for (typename HashMap::const_iterator it = map.begin(), end = map.end(); it != > end; ++it) { > if (WTF::NeedsTracing<Key>::value) { > CollectionBackingTraceTrait<KeyTraits::needsTracing, KeyTraits::isWeak, > false, Key, KeyTraits>::mark(v, it->key); > } > if (WTF::NeedsTracing<Value>::value) { > CollectionBackingTraceTrait<ValueTraits::needsTracing, ValueTraits::isWeak, > false, Value, ValueTraits>::mark(v, it->value); > } > } This would go through the loop even if nothing needs tracing. I don't think the compiler can optimize that away. The individual if's are not needed inside the loop, since the CollectionBackingTraceTrait<false,...> will expand to nothing. https://codereview.chromium.org/131803005/diff/1/Source/heap/Visitor.h#newcod... Source/heap/Visitor.h:438: static void mark(Visitor* v, const Vector& vector) On 2014/01/15 04:48:35, haraken wrote: > > v => visitor Done.
Thank for all the replies. LGTM.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erik.corry@gmail.com/131803005/150001
Failed to trigger a try job on linux_blink_rel HTTP Error 400: Bad Request
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erik.corry@gmail.com/131803005/320001
Retried try job too often on linux_blink_rel for step(s) blink_platform_unittests, webkit_lint, webkit_python_tests, webkit_tests, webkit_unit_tests, wtf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erik.corry@gmail.com/131803005/490001
Retried try job too often on linux_blink for step(s) wtf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erik.corry@gmail.com/131803005/490001
Retried try job too often on linux_blink for step(s) wtf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erik.corry@gmail.com/131803005/490001
Retried try job too often on win_layout for step(s) webkit_lint http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erik.corry@gmail.com/131803005/660002
Retried try job too often on win_layout for step(s) webkit_lint http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erik.corry@gmail.com/131803005/890002
Message was sent while issue was closed.
Change committed as 165932
Message was sent while issue was closed.
We're trying to get a build of chrome/win with clang going. On our
clang/win/debug bot, I'm seeing a diagnostic I don't understand:
..\..\third_party\WebKit\Source\wtf/HashTable.h(1233,75) : error(clang): cannot
pass object of non-trivial type 'ValueType' (aka
'WTF::KeyValuePair<WTF::AtomicString, WTF::AtomicString>') through variadic
function; call will abort at runtime [-Wnon-pod-varargs]
Allocator::template trace<ValueType, Traits>(visitor,
*element);
^
..\..\third_party\WebKit\Source\wtf/HashMap.h(155,67) : note(clang): in
instantiation of member function 'WTF::HashTable<WTF::AtomicString,
WTF::KeyValuePair<WTF::AtomicString, WTF::AtomicString>,
WTF::KeyValuePairKeyExtractor, WTF::CaseFoldingHash,
WTF::HashMapValueTraits<WTF::HashTraits<WTF::AtomicString>,
WTF::HashTraits<WTF::AtomicString> >, WTF::HashTraits<WTF::AtomicString>,
WTF::DefaultAllocator>::trace' requested here
void trace(typename Allocator::Visitor* visitor) {
m_impl.trace(visitor); }
^
..\..\third_party\WebKit\Source\wtf/HashMap.h(52,11) : note(clang): in
instantiation of member function 'WTF::HashMap<WTF::AtomicString,
WTF::AtomicString, WTF::CaseFoldingHash, WTF::HashTraits<WTF::AtomicString>,
WTF::HashTraits<WTF::AtomicString>, WTF::DefaultAllocator>::trace' requested
here
class HashMap {
^
1 error generated.
Do you know why I'd only see this on debug, not release? Does the diagnostic
make sense to you? I don't see the variadic function call, but cs.chromium.org
also can't tell me where "Allocator::template trace" is defined (does this have
to be so parametrized?) – so chances are this might be a real warning? No
existing bot complains about this though, and since clang/win is relatively new
it might be a compiler bug too.
Message was sent while issue was closed.
On 2014/07/25 16:58:38, Nico (away) wrote:
> We're trying to get a build of chrome/win with clang going. On our
> clang/win/debug bot, I'm seeing a diagnostic I don't understand:
>
> ..\..\third_party\WebKit\Source\wtf/HashTable.h(1233,75) : error(clang):
cannot
> pass object of non-trivial type 'ValueType' (aka
> 'WTF::KeyValuePair<WTF::AtomicString, WTF::AtomicString>') through variadic
> function; call will abort at runtime [-Wnon-pod-varargs]
> Allocator::template trace<ValueType, Traits>(visitor,
> *element);
> ^
> ..\..\third_party\WebKit\Source\wtf/HashMap.h(155,67) : note(clang): in
> instantiation of member function 'WTF::HashTable<WTF::AtomicString,
> WTF::KeyValuePair<WTF::AtomicString, WTF::AtomicString>,
> WTF::KeyValuePairKeyExtractor, WTF::CaseFoldingHash,
> WTF::HashMapValueTraits<WTF::HashTraits<WTF::AtomicString>,
> WTF::HashTraits<WTF::AtomicString> >, WTF::HashTraits<WTF::AtomicString>,
> WTF::DefaultAllocator>::trace' requested here
> void trace(typename Allocator::Visitor* visitor) {
> m_impl.trace(visitor); }
> ^
> ..\..\third_party\WebKit\Source\wtf/HashMap.h(52,11) : note(clang): in
> instantiation of member function 'WTF::HashMap<WTF::AtomicString,
> WTF::AtomicString, WTF::CaseFoldingHash, WTF::HashTraits<WTF::AtomicString>,
> WTF::HashTraits<WTF::AtomicString>, WTF::DefaultAllocator>::trace' requested
> here
> class HashMap {
> ^
> 1 error generated.
>
>
> Do you know why I'd only see this on debug, not release? Does the diagnostic
> make sense to you? I don't see the variadic function call, but
http://cs.chromium.org
> also can't tell me where "Allocator::template trace" is defined (does this
have
> to be so parametrized?) – so chances are this might be a real warning? No
> existing bot complains about this though, and since clang/win is relatively
new
> it might be a compiler bug too.
I'm heading off on vacation, so I don't have a lot of time to lookin into this.
I'm sure it won't actually call the varadic method at run time, since it's a
normal non-GCed HashMap that is involved, so none of the trace methods will be
called. However there are subtle differences between compilers around how
eagerly they instantiate templates, and it seems this compiler is instantiating
some template that will never be called. I don't think that's necessarily a
compiler bug, though it may be.
We have recently started putting the bodies of trace methods in #if's (see
https://codereview.chromium.org/403333002/ for examples) and if you can find out
which trace method is causing this to be instantiated then you may be able to do
the same.
For the varadic function you need to look for "...". I'm not sure it helps if
you find it. Alternatively you could look for the hashmap from case independent
AtomicString to AtomicString.
I think it needs to be so parameterized. I agree that it has become rather
complicated when we added the 6th template argument to HashMap (Allocator). The
trace method in question is in DefaultAllocator in DefaultAllocator.h
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/131803005/diff/890002/Source/heap/Heap.h File Source/heap/Heap.h (right): https://codereview.chromium.org/131803005/diff/890002/Source/heap/Heap.h#newc... Source/heap/Heap.h:1297: // MSVC supports it only in MSVC 2013. We're now on 2013. Can this be simplified now?
Message was sent while issue was closed.
https://codereview.chromium.org/131803005/diff/890002/Source/heap/Heap.h File Source/heap/Heap.h (right): https://codereview.chromium.org/131803005/diff/890002/Source/heap/Heap.h#newc... Source/heap/Heap.h:1297: // MSVC supports it only in MSVC 2013. On 2014/08/29 19:41:41, Nico (hiding) wrote: > We're now on 2013. Can this be simplified now? Thanks for the heads up! In that case we should be able to. I'll try it out. :)
Message was sent while issue was closed.
On 2014/09/01 06:34:35, Mads Ager (chromium) wrote: > https://codereview.chromium.org/131803005/diff/890002/Source/heap/Heap.h > File Source/heap/Heap.h (right): > > https://codereview.chromium.org/131803005/diff/890002/Source/heap/Heap.h#newc... > Source/heap/Heap.h:1297: // MSVC supports it only in MSVC 2013. > On 2014/08/29 19:41:41, Nico (hiding) wrote: > > We're now on 2013. Can this be simplified now? > > Thanks for the heads up! In that case we should be able to. I'll try it out. :) OK, the answer is that MSVC 2013 supposedly supports template aliases but when we use them we get a ton of internal compiler errors. I have resolved a bunch of them by explicitly expanding the template alias in a bunch of places where MSVC cannot deal with it, but at that point we don't gain anything. I'll leave this as is. MSVC is apparently not ready for this yet.
Message was sent while issue was closed.
On 2014/09/01 14:12:46, Mads Ager (chromium) wrote: > On 2014/09/01 06:34:35, Mads Ager (chromium) wrote: > > https://codereview.chromium.org/131803005/diff/890002/Source/heap/Heap.h > > File Source/heap/Heap.h (right): > > > > > https://codereview.chromium.org/131803005/diff/890002/Source/heap/Heap.h#newc... > > Source/heap/Heap.h:1297: // MSVC supports it only in MSVC 2013. > > On 2014/08/29 19:41:41, Nico (hiding) wrote: > > > We're now on 2013. Can this be simplified now? > > > > Thanks for the heads up! In that case we should be able to. I'll try it out. > :) > > OK, the answer is that MSVC 2013 supposedly supports template aliases but when > we use them we get a ton of internal compiler errors. I have resolved a bunch of > them by explicitly expanding the template alias in a bunch of places where MSVC > cannot deal with it, but at that point we don't gain anything. I'll leave this > as is. MSVC is apparently not ready for this yet. Here is the simple change that works on gcc and clang but not msvc. Fails with internal compiler errors. :(
Message was sent while issue was closed.
On 2014/09/01 14:14:50, Mads Ager (chromium) wrote: > On 2014/09/01 14:12:46, Mads Ager (chromium) wrote: > > On 2014/09/01 06:34:35, Mads Ager (chromium) wrote: > > > https://codereview.chromium.org/131803005/diff/890002/Source/heap/Heap.h > > > File Source/heap/Heap.h (right): > > > > > > > > > https://codereview.chromium.org/131803005/diff/890002/Source/heap/Heap.h#newc... > > > Source/heap/Heap.h:1297: // MSVC supports it only in MSVC 2013. > > > On 2014/08/29 19:41:41, Nico (hiding) wrote: > > > > We're now on 2013. Can this be simplified now? > > > > > > Thanks for the heads up! In that case we should be able to. I'll try it out. > > :) > > > > OK, the answer is that MSVC 2013 supposedly supports template aliases but when > > we use them we get a ton of internal compiler errors. I have resolved a bunch > of > > them by explicitly expanding the template alias in a bunch of places where > MSVC > > cannot deal with it, but at that point we don't gain anything. I'll leave this > > as is. MSVC is apparently not ready for this yet. > > Here is the simple change that works on gcc and clang but not msvc. Fails with > internal compiler errors. :( Ehm, *here* it is: https://codereview.chromium.org/526023003/
Message was sent while issue was closed.
> Ehm, *here* it is: https://codereview.chromium.org/526023003/ sadness, thanks for checking. Maybe file a bug with ms connect so that they can fix this, and consider updating the comment to not say "MSVC supports it only in MSVC 2013." ("MSVC 2013 claims to support this, but crashes when used, <link to msconnect bug>". |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
