Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
137483003:
Add template aliases to support both reference counting and garbage collection for a transition per… (Closed)
|
Created:
6 years, 11 months ago by Mads Ager (chromium) Modified:
6 years, 11 months ago Reviewers:
jamesr, Vyacheslav Egorov (Chromium), (unused - use chromium), zerny-chromium, Erik Corry, jbroman, abarth-chromium, haraken, darin (slow to review) CC:
blink-reviews, loislo+blink_chromium.org, Mads Ager (chromium), yurys+blink_chromium.org, abarth-chromium, adamk+blink_chromium.org, haraken, Inactive Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionAdd template aliases to support both reference counting and garbage collection for a transition period.
Template aliases are used in clang on which they are currently
supported. The aliases are simple enough that they can be
simulated with simple macros on gcc and msvc.
BUG=
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165803
Patch Set 1 #Patch Set 2 : Reupload with similarity=90 #
Total comments: 11
Patch Set 3 : Minor changes based on first comments. #Patch Set 4 : ENABLE(OILPAN) #
Total comments: 11
Patch Set 5 : Rebased and address Haraken's comments. #Patch Set 6 : Rebase #
Total comments: 2
Patch Set 7 : Address comment. #
Messages
Total messages: 19 (0 generated)
Based on discussions on blink-dev this is the best version that I have been able to create so far. I'm using template aliases to introduce the templated typedefs on clang where they are supported. For gcc and msvc I'm using stylized macros to introduce the same typedefs. Using template aliases on clang keeps us honest in the sense that we cannot do anything else than simple typedefs with the macros here. If we did, we could not make it work both with template aliases and with macros. I have added the people who raised their voice on the blink-dev thread to the list of reviewers. What do you think of this compromise?
lgtm https://codereview.chromium.org/137483003/diff/20001/Source/heap/Handle.h File Source/heap/Handle.h (right): https://codereview.chromium.org/137483003/diff/20001/Source/heap/Handle.h#new... Source/heap/Handle.h:421: #if ENABLE_OILPAN Should not this be #if ENABLE(OILPAN) ? https://codereview.chromium.org/137483003/diff/20001/Source/heap/Handle.h#new... Source/heap/Handle.h:423: #if COMPILER(CLANG) While this is indeed very robust compared to defines, I think it might be better to use the same code on all platforms we deploy to. I would say that OilPan move most likely will be complete before we will find ourselves in situation where we can use template aliases on all platforms. https://codereview.chromium.org/137483003/diff/20001/Source/heap/Handle.h#new... Source/heap/Handle.h:437: #define PassRefPtrWillBePtr Ptr There are two lists which might be a hassle to keep in sync (also conditional define is enormous in size. How about #if ENABLE(OILPAN) # define SWITCH(Current, WillBe) WillBe #else # define SWITCH(Current, WillBe) Current #endif #define PassRefPtrWillBePtr SWITCH(PassRefPtr, Ptr) #define OwnPtrWillBeMember SWITCH(OwnPtr, Member) // ... etc ... #undef SWITCH I think this is easier to read and maintain. If you decide to keep CLANG variant too we can think about unifying all of them into this. https://codereview.chromium.org/137483003/diff/20001/Source/heap/HeapTest.cpp File Source/heap/HeapTest.cpp (right): https://codereview.chromium.org/137483003/diff/20001/Source/heap/HeapTest.cpp... Source/heap/HeapTest.cpp:586: #if ENABLE_OILPAN should it be ENABLE(OILPAN) (and likewise in other parts of the file?)
I'm not an expert on this, but some comments nonetheless. https://codereview.chromium.org/137483003/diff/20001/Source/heap/Handle.h File Source/heap/Handle.h (right): https://codereview.chromium.org/137483003/diff/20001/Source/heap/Handle.h#new... Source/heap/Handle.h:424: template<typename T> using PassRefPtrWillBePtr = Ptr<T>; Why use template aliases on Clang if we're still going to be using preprocessor macros for other compilers? This would be even more weird if other reviewers insist on SHOUTY_CASE macros. Also, maybe consider whether this condition should use a wtf/Compiler.h macro (e.g. COMPILER_SUPPORTS(CXX_TEMPLATE_ALIASES)). Not sure this is the right place for assumptions about which features which compilers support. https://codereview.chromium.org/137483003/diff/20001/Source/heap/Handle.h#new... Source/heap/Handle.h:491: template<typename T> PassRefPtr<T> adoptRefWillBeNoop(T* ptr) { return adoptRef(ptr); } nit: Above this is declared as returning PassRefPtrWillBePtr<T>; here it is declared as returning PassRefPtr<T>. On the other hand, adoptPtrWillBeNoop has the same signature here and above. Probably better to be consistent about using the aliases or not in these declarations.
https://codereview.chromium.org/137483003/diff/20001/Source/heap/Handle.h File Source/heap/Handle.h (right): https://codereview.chromium.org/137483003/diff/20001/Source/heap/Handle.h#new... Source/heap/Handle.h:421: #if ENABLE_OILPAN On 2014/01/15 14:21:10, Vyacheslav Egorov wrote: > Should not this be #if ENABLE(OILPAN) ? Yes, thanks. https://codereview.chromium.org/137483003/diff/20001/Source/heap/Handle.h#new... Source/heap/Handle.h:423: #if COMPILER(CLANG) On 2014/01/15 14:21:10, Vyacheslav Egorov wrote: > While this is indeed very robust compared to defines, I think it might be better > to use the same code on all platforms we deploy to. > > I would say that OilPan move most likely will be complete before we will find > ourselves in situation where we can use template aliases on all platforms. I agree with that assessment. The reason for using the template aliases here was that it will keep us honest and makes it clear that these are just templated typedefs. It will keep us from doing nasty things with the macros that is not just a typedef. https://codereview.chromium.org/137483003/diff/20001/Source/heap/Handle.h#new... Source/heap/Handle.h:424: template<typename T> using PassRefPtrWillBePtr = Ptr<T>; On 2014/01/15 14:37:09, jbroman wrote: > Why use template aliases on Clang if we're still going to be using preprocessor > macros for other compilers? This would be even more weird if other reviewers > insist on SHOUTY_CASE macros. > > Also, maybe consider whether this condition should use a wtf/Compiler.h macro > (e.g. COMPILER_SUPPORTS(CXX_TEMPLATE_ALIASES)). Not sure this is the right place > for assumptions about which features which compilers support. To keep us honest so that we cannot do anything nasty with the macros that is not just a typedef. This approach only works if we allow the CamelCase macros. https://codereview.chromium.org/137483003/diff/20001/Source/heap/Handle.h#new... Source/heap/Handle.h:437: #define PassRefPtrWillBePtr Ptr On 2014/01/15 14:21:10, Vyacheslav Egorov wrote: > There are two lists which might be a hassle to keep in sync (also conditional > define is enormous in size. How about > > #if ENABLE(OILPAN) > # define SWITCH(Current, WillBe) WillBe > #else > # define SWITCH(Current, WillBe) Current > #endif > > #define PassRefPtrWillBePtr SWITCH(PassRefPtr, Ptr) > #define OwnPtrWillBeMember SWITCH(OwnPtr, Member) > // ... etc ... > > #undef SWITCH > > I think this is easier to read and maintain. > > If you decide to keep CLANG variant too we can think about unifying all of them > into this. That can be done and it sounds like a good idea. I'll do that if we can agree that this is a reasonable compromise. https://codereview.chromium.org/137483003/diff/20001/Source/heap/Handle.h#new... Source/heap/Handle.h:491: template<typename T> PassRefPtr<T> adoptRefWillBeNoop(T* ptr) { return adoptRef(ptr); } On 2014/01/15 14:37:09, jbroman wrote: > nit: Above this is declared as returning PassRefPtrWillBePtr<T>; here it is > declared as returning PassRefPtr<T>. On the other hand, adoptPtrWillBeNoop has > the same signature here and above. Probably better to be consistent about using > the aliases or not in these declarations. Yes, that was a typo that happened to be the right thing here. Thanks. :)
Alternative using typedefs inside of the oilpan types: https://codereview.chromium.org/130493003/ That would work for me too. What do you guys think? Any strong preferences?
LGTM. https://codereview.chromium.org/137483003/diff/140001/Source/heap/Handle.h File Source/heap/Handle.h (right): https://codereview.chromium.org/137483003/diff/140001/Source/heap/Handle.h#ne... Source/heap/Handle.h:416: // enable_oilpan compile-time flag. enable_oilpan => ENABLE(OILPAN) https://codereview.chromium.org/137483003/diff/140001/Source/heap/Handle.h#ne... Source/heap/Handle.h:432: template<typename T> using OwnPtrWillBeMember = Member<T>; We'll also need OwnPtrWillBePersistent. https://codereview.chromium.org/137483003/diff/140001/Source/wtf/Ptr.h File Source/wtf/Ptr.h (right): https://codereview.chromium.org/137483003/diff/140001/Source/wtf/Ptr.h#newcode13 Source/wtf/Ptr.h:13: * THIS SOFTWARE IS PROVIDED BY APPLE INC. ``AS IS'' AND ANY Not APPLE :) Please use the copyright in heap/Heap.h. https://codereview.chromium.org/137483003/diff/140001/Source/wtf/Ptr.h#newcode30 Source/wtf/Ptr.h:30: // interface (get, clear) of other pointer types such as RefPtr, Are compilers smart enough to optimize Ptr operations just like raw pointer operations? Maybe we want to make sure of it before starting a lot of edits. https://codereview.chromium.org/137483003/diff/140001/Source/wtf/Ptr.h#newcode66 Source/wtf/Ptr.h:66: T& operator*() const { return *m_ptr; } Don't we need: const T& operator*() const; ?
https://codereview.chromium.org/137483003/diff/140001/Source/heap/Handle.h File Source/heap/Handle.h (right): https://codereview.chromium.org/137483003/diff/140001/Source/heap/Handle.h#ne... Source/heap/Handle.h:416: // enable_oilpan compile-time flag. On 2014/01/16 05:21:22, haraken wrote: > > enable_oilpan => ENABLE(OILPAN) I rewrote this to just say 'based on a compile-time flag' instead of mentioning the flag which is clear from the code below. https://codereview.chromium.org/137483003/diff/140001/Source/heap/Handle.h#ne... Source/heap/Handle.h:432: template<typename T> using OwnPtrWillBeMember = Member<T>; On 2014/01/16 05:21:22, haraken wrote: > > We'll also need OwnPtrWillBePersistent. Yes, and there will be more than that. I added the ones that I needed for the example in HeapTest.cpp. We should add more as we need them and add tests for them as well. https://codereview.chromium.org/137483003/diff/140001/Source/wtf/Ptr.h File Source/wtf/Ptr.h (right): https://codereview.chromium.org/137483003/diff/140001/Source/wtf/Ptr.h#newcode13 Source/wtf/Ptr.h:13: * THIS SOFTWARE IS PROVIDED BY APPLE INC. ``AS IS'' AND ANY On 2014/01/16 05:21:22, haraken wrote: > > Not APPLE :) Please use the copyright in heap/Heap.h. Heh! Without thinking I copied this from RefPtr.h. Fixed! https://codereview.chromium.org/137483003/diff/140001/Source/wtf/Ptr.h#newcode30 Source/wtf/Ptr.h:30: // interface (get, clear) of other pointer types such as RefPtr, On 2014/01/16 05:21:22, haraken wrote: > > Are compilers smart enough to optimize Ptr operations just like raw pointer > operations? Maybe we want to make sure of it before starting a lot of edits. Yes, they should be. The issue that we saw when we used Result was that passing and returning one of these by value was not properly optimized by some compilers. However, we will mostly be using these as members of classes and extract the pointer and pass it around directly. We will also use them where a similar pointer class such as PassRefPtr was already used but then the compiler will be able to do as well as it did before. I would not expect performance regressions based on this. https://codereview.chromium.org/137483003/diff/140001/Source/wtf/Ptr.h#newcode66 Source/wtf/Ptr.h:66: T& operator*() const { return *m_ptr; } On 2014/01/16 05:21:22, haraken wrote: > > Don't we need: > > const T& operator*() const; > > ? I don't think so, no. T itself can be something like 'const Node' here and you can always use a 'Node' where a 'const Node' is expected (but not the other way around).
https://codereview.chromium.org/137483003/diff/140001/Source/wtf/Ptr.h File Source/wtf/Ptr.h (right): https://codereview.chromium.org/137483003/diff/140001/Source/wtf/Ptr.h#newcode30 Source/wtf/Ptr.h:30: // interface (get, clear) of other pointer types such as RefPtr, On 2014/01/16 07:04:15, Mads Ager (chromium) wrote: > On 2014/01/16 05:21:22, haraken wrote: > > > > Are compilers smart enough to optimize Ptr operations just like raw pointer > > operations? Maybe we want to make sure of it before starting a lot of edits. > > Yes, they should be. The issue that we saw when we used Result was that passing > and returning one of these by value was not properly optimized by some > compilers. However, we will mostly be using these as members of classes and > extract the pointer and pass it around directly. We will also use them where a > similar pointer class such as PassRefPtr was already used but then the compiler > will be able to do as well as it did before. I would not expect performance > regressions based on this. Makes sense. Thanks for the clarification.
Source/wtf LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ager@chromium.org/137483003/260001
lgtm https://codereview.chromium.org/137483003/diff/260001/Source/heap/Handle.h File Source/heap/Handle.h (right): https://codereview.chromium.org/137483003/diff/260001/Source/heap/Handle.h#ne... Source/heap/Handle.h:214: operator Ptr<T>() const { return Ptr<T>(m_raw); } You should be able to just write "return m_raw" here.
Retried try job too often on linux_blink_rel for step(s) webkit_lint http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...
https://codereview.chromium.org/137483003/diff/260001/Source/heap/Handle.h File Source/heap/Handle.h (right): https://codereview.chromium.org/137483003/diff/260001/Source/heap/Handle.h#ne... Source/heap/Handle.h:214: operator Ptr<T>() const { return Ptr<T>(m_raw); } On 2014/01/24 19:11:53, Erik Corry wrote: > You should be able to just write "return m_raw" here. Yes, thanks! Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ager@chromium.org/137483003/440001
Retried try job too often on linux_blink_rel for step(s) webkit_tests 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/ager@chromium.org/137483003/440001
Retried try job too often on linux_blink_rel for step(s) webkit_tests 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/ager@chromium.org/137483003/440001
Message was sent while issue was closed.
Change committed as 165803 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
