Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10790149:
Add awesome_ptr<>. (Closed)
|
Created:
8 years, 5 months ago by awong Modified:
8 years ago Reviewers:
Avi (use Gerrit), dglazkov, Ryan Sleevi, srvasude1, dmichael (off chromium), jln (very slow on Chromium), darin (slow to review), levin, Scott Hess - ex-Googler, willchan no longer on Chromium CC:
chromium-reviews, erikwright (departed), brettw-cc_chromium.org, Ami GONE FROM CHROMIUM, kinuko+watch, hans, tzik+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd awesome_ptr<>.
awesome_ptr<> provides instrumentation in the codebase that allows for
small-footprint, real-usage testing scenarios. Long term, it can help
improve the debugging chops of maintainers on the chromium project.
Data is collected via the crash reporting mechanism.
awesome_ptr<> is safe for multi-threaded usage.
BUG=none
TEST=int main(void) {char* c = (char*)0x1;awesome_ptr<char> p(c); if(p) printf("%p\n", (char*)p); return 0; } compiled and printed 0x1.
Patch Set 1 #
Total comments: 20
Patch Set 2 : address comments, make const correct and 64-bit safe. #Patch Set 3 : Fixed type conversion. #
Total comments: 10
Patch Set 4 : Address shess's comments. Add unittests and fix readability issue with single gotos. #
Total comments: 8
Patch Set 5 : Fix test names. #Patch Set 6 : Add missing .h, and fix bug that levin@ spotted. #Patch Set 7 : stack allocated for speed (per levin@) #
Total comments: 5
Messages
Total messages: 19 (0 generated)
LATM https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_ptr.h File base/memory/awesome_ptr.h (right): https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_pt... base/memory/awesome_ptr.h:8: // small-footpring, real-usage testing scenarios. Long term, it can help nit:footprint https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_pt... base/memory/awesome_ptr.h:18: awesome_ptr(T* ptr) : ptr_(ptr) {} shouldn't this be: template <typename Y> awesome_ptr(Y* ptr) : ptr_(reinterpret_cast<T*>(ptr)) {} ? Letting clients use any type would be even more awesome. https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_pt... base/memory/awesome_ptr.h:48: break; I didn't really read this part, but I assume it's all kosher.
https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_ptr.h File base/memory/awesome_ptr.h (right): https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_pt... base/memory/awesome_ptr.h:8: // small-footpring, real-usage testing scenarios. Long term, it can help s/footpring/footprint/ https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_pt... base/memory/awesome_ptr.h:18: awesome_ptr(T* ptr) : ptr_(ptr) {} indentation, |explicit| https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_pt... base/memory/awesome_ptr.h:42: ptr_ = (T*)((int)ptr_ | (1 << (rand() % sizeof(int)))); Can we abstract out the |(1 << (rand() % sizeof(int)))|? Seems like it's used in both cases 1 and 2. Perhaps a macro? https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_pt... base/memory/awesome_ptr.h:47: ptr_ = ptr_ | 0x1; This is subsumed by case 1. Can we find more awesomeness so we don't waste this case?
I would not be exaggerating one bit if I state that this patch is full of awesome.
https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_ptr.h File base/memory/awesome_ptr.h (right): https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_pt... base/memory/awesome_ptr.h:18: awesome_ptr(T* ptr) : ptr_(ptr) {} On 2012/07/24 18:32:04, Avi wrote: > indentation, |explicit| I disagree, allowing implicit conversion is awesome. (Note the implicit conversion to T* also). https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_pt... base/memory/awesome_ptr.h:19: operator T*() const { On 2nd look, this could also be a template so you can convert to any pointer. https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_pt... base/memory/awesome_ptr.h:25: return be_awesome() ? true : ptr_; true and ptr_ are different types. Are you sure the compiler can handle this level of awesomeness?
+willchan and darin for OWNERS https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_ptr.h File base/memory/awesome_ptr.h (right): https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_pt... base/memory/awesome_ptr.h:8: // small-footpring, real-usage testing scenarios. Long term, it can help On 2012/07/24 18:30:45, dmichael wrote: > nit:footprint Done. https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_pt... base/memory/awesome_ptr.h:8: // small-footpring, real-usage testing scenarios. Long term, it can help On 2012/07/24 18:32:04, Avi wrote: > s/footpring/footprint/ Done. https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_pt... base/memory/awesome_ptr.h:18: awesome_ptr(T* ptr) : ptr_(ptr) {} On 2012/07/24 18:32:04, Avi wrote: > indentation, |explicit| Good catch on the explicit. https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_pt... base/memory/awesome_ptr.h:18: awesome_ptr(T* ptr) : ptr_(ptr) {} On 2012/07/24 18:30:45, dmichael wrote: > shouldn't this be: > template <typename Y> > awesome_ptr(Y* ptr) : ptr_(reinterpret_cast<T*>(ptr)) {} > ? > Letting clients use any type would be even more awesome. I like it. Used a c-style case though since it's more accepting across kinds. https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_pt... base/memory/awesome_ptr.h:18: awesome_ptr(T* ptr) : ptr_(ptr) {} On 2012/07/24 19:04:35, dmichael wrote: > On 2012/07/24 18:32:04, Avi wrote: > > indentation, |explicit| > I disagree, allowing implicit conversion is awesome. (Note the implicit > conversion to T* also). No no...we have to follow the style guide. https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_pt... base/memory/awesome_ptr.h:19: operator T*() const { On 2012/07/24 19:04:35, dmichael wrote: > On 2nd look, this could also be a template so you can convert to any pointer. Unfortunately template inference only works for function arguments. Sad. https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_pt... base/memory/awesome_ptr.h:25: return be_awesome() ? true : ptr_; On 2012/07/24 19:04:35, dmichael wrote: > true and ptr_ are different types. Are you sure the compiler can handle this > level of awesomeness? Agh. Good catch. fixed the type system. https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_pt... base/memory/awesome_ptr.h:42: ptr_ = (T*)((int)ptr_ | (1 << (rand() % sizeof(int)))); On 2012/07/24 18:32:04, Avi wrote: > Can we abstract out the |(1 << (rand() % sizeof(int)))|? Seems like it's used in > both cases 1 and 2. Perhaps a macro? Done. Also sped up the macro by using float instead of double for extra performance. https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_pt... base/memory/awesome_ptr.h:47: ptr_ = ptr_ | 0x1; On 2012/07/24 18:32:04, Avi wrote: > This is subsumed by case 1. Can we find more awesomeness so we don't waste this > case? It's not quite the same if you think about memory alignment on modern architectures. Switched it to & to exploit the memory alignment. Useful in RISC architectures. https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_pt... base/memory/awesome_ptr.h:48: break; On 2012/07/24 18:30:45, dmichael wrote: > I didn't really read this part, but I assume it's all kosher. vegan really. No animal byproducts used.
LATM; my concerns were all addressed.
Needs unit tests. https://chromiumcodereview.appspot.com/10790149/diff/6002/base/memory/awesome... File base/memory/awesome_ptr.h (right): https://chromiumcodereview.appspot.com/10790149/diff/6002/base/memory/awesome... base/memory/awesome_ptr.h:4: Header guards. https://chromiumcodereview.appspot.com/10790149/diff/6002/base/memory/awesome... base/memory/awesome_ptr.h:13: // awesome_ptr<> is safe for multi-threaded usage. I think #define private public would make it more broadly useful. https://chromiumcodereview.appspot.com/10790149/diff/6002/base/memory/awesome... base/memory/awesome_ptr.h:15: #define MAKE_MASK(type) (((type)1) << (rand() % sizeof(float))) This potentially calls rand() a lot. Could you make it static for speed? https://chromiumcodereview.appspot.com/10790149/diff/6002/base/memory/awesome... base/memory/awesome_ptr.h:36: if (!was_awesome) { Chromium style would be if (!was_awesome) return false;, and just return true at the end. Though I could see leaving it as-is if you changed the label to aaawww___yyyeeeaaa: https://chromiumcodereview.appspot.com/10790149/diff/6002/base/memory/awesome... base/memory/awesome_ptr.h:45: ptr_ = (T*)((unsigned long)ptr_ | MAKE_MASK(unsigned long)); Why not uintptr_t? Too much footprint?
PTAL https://chromiumcodereview.appspot.com/10790149/diff/6002/base/memory/awesome... File base/memory/awesome_ptr.h (right): https://chromiumcodereview.appspot.com/10790149/diff/6002/base/memory/awesome... base/memory/awesome_ptr.h:4: On 2012/07/24 19:22:15, shess wrote: > Header guards. Done. https://chromiumcodereview.appspot.com/10790149/diff/6002/base/memory/awesome... base/memory/awesome_ptr.h:13: // awesome_ptr<> is safe for multi-threaded usage. On 2012/07/24 19:22:15, shess wrote: > I think #define private public would make it more broadly useful. True, but that can lead to ODR violations which would be unsafe and might cause non-deterministic behavior. Code should be deterministicaly non-deterministic to preserve readability. https://chromiumcodereview.appspot.com/10790149/diff/6002/base/memory/awesome... base/memory/awesome_ptr.h:15: #define MAKE_MASK(type) (((type)1) << (rand() % sizeof(float))) On 2012/07/24 19:22:15, shess wrote: > This potentially calls rand() a lot. Could you make it static for speed? Yes, but that optimization comes with the tradeoff of shrinking the surface of awesomeness. https://chromiumcodereview.appspot.com/10790149/diff/6002/base/memory/awesome... base/memory/awesome_ptr.h:36: if (!was_awesome) { On 2012/07/24 19:22:15, shess wrote: > Chromium style would be if (!was_awesome) return false;, and just return true at > the end. Though I could see leaving it as-is if you changed the label to > aaawww___yyyeeeaaa: Good point. However, aaawww___yyyeeeaaa doesn't express the proper disappointment at not getting to be awesome. But the style problem is a strong one. The core problem is that single gotos are unreadable since the reader is left thinking "what if". Rectified by adding an else statement. https://chromiumcodereview.appspot.com/10790149/diff/6002/base/memory/awesome... base/memory/awesome_ptr.h:45: ptr_ = (T*)((unsigned long)ptr_ | MAKE_MASK(unsigned long)); On 2012/07/24 19:22:15, shess wrote: > Why not uintptr_t? Too much footprint? Thought about it...then realized it was c99 and might not be friendly to MSVC.
https://chromiumcodereview.appspot.com/10790149/diff/3002/base/memory/awesome... File base/memory/awesome_ptr_unittest.cc (right): https://chromiumcodereview.appspot.com/10790149/diff/3002/base/memory/awesome... base/memory/awesome_ptr_unittest.cc:15: TEST(AwesomePtrTest, PointerFunctionality) { This tests the bool conversion, so I'd expect it to be called BoolFunctionality... https://chromiumcodereview.appspot.com/10790149/diff/3002/base/memory/awesome... base/memory/awesome_ptr_unittest.cc:21: TEST(AwesomePtrTest, BoolFunctionality) { ...so this would need to be called something else.
https://chromiumcodereview.appspot.com/10790149/diff/3002/base/memory/awesome... File base/memory/awesome_ptr_unittest.cc (right): https://chromiumcodereview.appspot.com/10790149/diff/3002/base/memory/awesome... base/memory/awesome_ptr_unittest.cc:15: TEST(AwesomePtrTest, PointerFunctionality) { On 2012/07/24 20:52:14, Avi wrote: > This tests the bool conversion, so I'd expect it to be called > BoolFunctionality... Doh! cut/paste error. https://chromiumcodereview.appspot.com/10790149/diff/3002/base/memory/awesome... base/memory/awesome_ptr_unittest.cc:21: TEST(AwesomePtrTest, BoolFunctionality) { On 2012/07/24 20:52:14, Avi wrote: > ...so this would need to be called something else. fixed.
LATM. guess you just need an OWNERS review. https://chromiumcodereview.appspot.com/10790149/diff/3002/base/base.gyp File base/base.gyp (right): https://chromiumcodereview.appspot.com/10790149/diff/3002/base/base.gyp#newco... base/base.gyp:412: 'memory/awesome_ptr_unittest.cc', Missing the .h in some gyp file or other, then. https://chromiumcodereview.appspot.com/10790149/diff/3002/base/memory/awesome... File base/memory/awesome_ptr.h (right): https://chromiumcodereview.appspot.com/10790149/diff/3002/base/memory/awesome... base/memory/awesome_ptr.h:12: namespace base { [Yearns for namespace basically, or perhaps totally.]
okay, last revision. Per offline discussion with levin@, added in stack allocated version of ptr on return using placement new. You know...for speed. https://chromiumcodereview.appspot.com/10790149/diff/3002/base/base.gyp File base/base.gyp (right): https://chromiumcodereview.appspot.com/10790149/diff/3002/base/base.gyp#newco... base/base.gyp:412: 'memory/awesome_ptr_unittest.cc', On 2012/07/24 21:06:40, shess wrote: > Missing the .h in some gyp file or other, then. Done. https://chromiumcodereview.appspot.com/10790149/diff/3002/base/memory/awesome... File base/memory/awesome_ptr.h (right): https://chromiumcodereview.appspot.com/10790149/diff/3002/base/memory/awesome... base/memory/awesome_ptr.h:12: namespace base { On 2012/07/24 21:06:40, shess wrote: > [Yearns for namespace basically, or perhaps totally.] totally.
https://chromiumcodereview.appspot.com/10790149/diff/1007/base/memory/awesome... File base/memory/awesome_ptr.h (right): https://chromiumcodereview.appspot.com/10790149/diff/1007/base/memory/awesome... base/memory/awesome_ptr.h:32: new (ptr) T*((T*)ptr_); // stack allocated for speed. Thanks. I think this makes a lot more sense now. I think everyone will appreciate the speed up afforded by this.
I don't think this new class has sufficient test coverage.
https://chromiumcodereview.appspot.com/10790149/diff/1007/base/memory/awesome... File base/memory/awesome_ptr.h (right): https://chromiumcodereview.appspot.com/10790149/diff/1007/base/memory/awesome... base/memory/awesome_ptr.h:46: goto awww; drive-by nit: I'm a little concerned about the goto here, because it could be easily misused... What about using longjmp instead?
https://chromiumcodereview.appspot.com/10790149/diff/1007/base/memory/awesome... File base/memory/awesome_ptr.h (right): https://chromiumcodereview.appspot.com/10790149/diff/1007/base/memory/awesome... base/memory/awesome_ptr.h:32: new (ptr) T*((T*)ptr_); // stack allocated for speed. shouldn't it be new (ptr) T*(const_cast<T*>(ptr_)); ?
Another neat trick: you should make use of non canonical AMD64 addresses, that way you can #GP and make use of the OS crash handler reporting and bypass breakpad! https://chromiumcodereview.appspot.com/10790149/diff/1007/base/memory/awesome... File base/memory/awesome_ptr.h (right): https://chromiumcodereview.appspot.com/10790149/diff/1007/base/memory/awesome... base/memory/awesome_ptr.h:46: goto awww; On 2012/07/25 02:37:07, Ryan Sleevi wrote: > drive-by nit: I'm a little concerned about the goto here, because it could be > easily misused... > > What about using longjmp instead? Be careful though, you should actually use siglongjmp and do the sigsetjmp in the constructor. That way, an awesome pointer can be saved in a global variable and used in a signal handler without the risk of a custom sigsegv handler being blocked. This could become handy in content/sandbox_init_linux.cc:CrashSIGSYS_Handler() :)
https://chromiumcodereview.appspot.com/10790149/diff/1007/base/memory/awesome... File base/memory/awesome_ptr.h (right): https://chromiumcodereview.appspot.com/10790149/diff/1007/base/memory/awesome... base/memory/awesome_ptr.h:60: ptr_ = (T*)((unsigned long)ptr_ & ~(MAKE_MASK(unsigned long))); Small nit: There is no break statement here. The switch statement should either have no break statements or each case should have a break statement for consistency. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
