Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14427006:
Introduce a Factory for Declarative API objects that performs deduping (Closed)
|
Created:
7 years, 8 months ago by battre Modified:
7 years, 7 months ago Reviewers:
vabr (Chromium) CC:
chromium-reviews, Aaron Boodman, chromium-apps-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionIntroduce a Factory for Declarative API objects that performs deduping
Many actions in the declarative WebRequest API are identical, e.g. because they
redirect to the same destination. This CL reduces the likelihood that we
allocate them multiple times, by adding a MRU cache. If a previously allocated
object can be found in the cache, it is recycled.
BUG=234232
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=197361
Patch Set 1 #
Total comments: 19
Patch Set 2 : Generalize DeduplicatingFactory so that it can be used for ContentAttributes as well #
Total comments: 4
Patch Set 3 : Addressed Vaclav's comments #Patch Set 4 : Addressed vaclav's comments #
Total comments: 2
Patch Set 5 : Fixed failing tests #Patch Set 6 : Merged with ToT #
Messages
Total messages: 12 (0 generated)
Hi Vaclav, can you please take a look at this CL? Thanks, Dominic
Thanks, Dominic. I left some comments. Do we wait with using the DedupingFactory for actions because it is not clear whether the saved memory is worth the added processing? Or do you have other reasons? Vaclav https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/declarative/deduping_factory.h (right): https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/declarative/deduping_factory.h:23: // kind. These objects need to be immutable, refcounted objects that are derived Maybe we should emphasise that this class does not require BaseClassT to be thread-safe refcounted. https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/declarative/deduping_factory.h:84: size_t max_number_prototypes_; This should be at least const, but I would even recommend to make it a template argument. That way it becomes a compile-time constant, will take no space, and possibly allow more optimizations. https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/declarative/deduping_factory.h:104: DCHECK(factory_methods_.find(instance_type) == factory_methods_.end()); nit: !ContainsKey(factory_methods_, instance_type) https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/declarative/deduping_factory.h:131: if (parameterized_types_.find(instance_type) == parameterized_types_.end()) { nit: !ContainsKey(parametrized_types_, instance_type) https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/declarative/deduping_factory.h:166: prototypes.pop_front(); If this goes just before the push_back (adjusting "length > max_number_prototypes_" to "length >= max_number_prototypes_"), we keep the prototypes' length bounded by max_number_prototypes_ all times. https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/declarative/deduping_factory_unittest.cc (right): https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/declarative/deduping_factory_unittest.cc:22: virtual Type GetType() const = 0; This is just an accessor of a constant member variable, so a IMO* better way to implement is as a constructor argument of the base class: class BaseClass ... { public: Type type() const { return type_; } // instead of GetType protected: explicit BaseClass(Type type) : type_(type) {} private: const Type type_; } I know this is just a unit test, but it may be copy-pasted to a production code if somebody is lazy implementing Equals. :) * Also, I learned that from Jeffrey during refactoring. https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/declarative/deduping_factory_unittest.cc:22: virtual Type GetType() const = 0; If GetType() (or type()) is only used in Equals, it should be protected.
https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/declarative/deduping_factory_unittest.cc (right): https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/declarative/deduping_factory_unittest.cc:22: virtual Type GetType() const = 0; On 2013/04/23 12:22:12, vabr (Chromium) wrote: > If GetType() (or type()) is only used in Equals, it should be protected. And if GetType/type is protected, then so should be Type.
Please take another look. Also note that I did further changes in Change Set 2. Thanks, Dominic https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/declarative/deduping_factory.h (right): https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/declarative/deduping_factory.h:23: // kind. These objects need to be immutable, refcounted objects that are derived On 2013/04/23 12:22:12, vabr (Chromium) wrote: > Maybe we should emphasise that this class does not require BaseClassT to be > thread-safe refcounted. Done. https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/declarative/deduping_factory.h:84: size_t max_number_prototypes_; On 2013/04/23 12:22:12, vabr (Chromium) wrote: > This should be at least const, but I would even recommend to make it a template > argument. That way it becomes a compile-time constant, will take no space, and > possibly allow more optimizations. I have made it const (even though it is pretty uncommon to have const members in Chrome in my opinion). I had code to make it a template argument but removed it because the syntax was too cumbersome and I felt that the advantages do not outweigh the ugly syntax. https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/declarative/deduping_factory.h:104: DCHECK(factory_methods_.find(instance_type) == factory_methods_.end()); On 2013/04/23 12:22:12, vabr (Chromium) wrote: > nit: !ContainsKey(factory_methods_, instance_type) Done. https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/declarative/deduping_factory.h:131: if (parameterized_types_.find(instance_type) == parameterized_types_.end()) { On 2013/04/23 12:22:12, vabr (Chromium) wrote: > nit: !ContainsKey(parametrized_types_, instance_type) Done. https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/declarative/deduping_factory.h:166: prototypes.pop_front(); On 2013/04/23 12:22:12, vabr (Chromium) wrote: > If this goes just before the push_back (adjusting "length > > max_number_prototypes_" to "length >= max_number_prototypes_"), we keep the > prototypes' length bounded by max_number_prototypes_ all times. Done. https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/declarative/deduping_factory_unittest.cc (right): https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/declarative/deduping_factory_unittest.cc:22: virtual Type GetType() const = 0; On 2013/04/23 12:22:12, vabr (Chromium) wrote: > This is just an accessor of a constant member variable, so a IMO* better way to > implement is as a constructor argument of the base class: > class BaseClass ... { > public: > Type type() const { return type_; } // instead of GetType > protected: > explicit BaseClass(Type type) : type_(type) {} > private: > const Type type_; > } > > I know this is just a unit test, but it may be copy-pasted to a production code > if somebody is lazy implementing Equals. :) > > > * Also, I learned that from Jeffrey during refactoring. Done. https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/declarative/deduping_factory_unittest.cc:22: virtual Type GetType() const = 0; On 2013/04/23 12:30:43, vabr (Chromium) wrote: > On 2013/04/23 12:22:12, vabr (Chromium) wrote: > > If GetType() (or type()) is only used in Equals, it should be protected. > > And if GetType/type is protected, then so should be Type. If we protect GetType(), Foo::Equals cannot call it anymore.
Hi Dominic, Thanks, LGTM with comments. The changes between PS1 and PS2 look fine, although I'm not sure why you introduced the instance_type string in the FactoryMethod. Do you have a particular use-case in mind, or is it just for future? If the latter, then I would recommend against it -- smaller arguments list make for faster execution, and adding a parameter in the future should not be hard. Cheers, Vaclav https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/declarative/deduping_factory.h (right): https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/declarative/deduping_factory.h:84: size_t max_number_prototypes_; On 2013/04/23 14:20:50, battre wrote: > On 2013/04/23 12:22:12, vabr (Chromium) wrote: > > This should be at least const, but I would even recommend to make it a > template > > argument. That way it becomes a compile-time constant, will take no space, and > > possibly allow more optimizations. > > I have made it const (even though it is pretty uncommon to have const members in > Chrome in my opinion). > I had code to make it a template argument but removed it > because the syntax was too cumbersome and I felt that the advantages do not > outweigh the ugly syntax. I don't mean to be a pain in the ass, so feel free to ignore this comment. :) What exactly was cumbersome? It already is a template, so < and > are already there, there would be one line less in the definition, and the max_number would be just on another place in the instantiation, no? Strange syntax is unfortunately the burden of C++, and should not drive the design decision. A good reason against template parameters is that they generate too many copies of the code, trading mostly speed for binary size. If we avoided templates for syntax, hardly anybody would use them. :) https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/declarative/deduping_factory_unittest.cc (right): https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/declarative/deduping_factory_unittest.cc:22: virtual Type GetType() const = 0; On 2013/04/23 14:20:50, battre wrote: > On 2013/04/23 12:30:43, vabr (Chromium) wrote: > > On 2013/04/23 12:22:12, vabr (Chromium) wrote: > > > If GetType() (or type()) is only used in Equals, it should be protected. > > > > And if GetType/type is protected, then so should be Type. > > If we protect GetType(), Foo::Equals cannot call it anymore. Are you sure? All protected parts of a base class are accessible to children. See http://ideone.com/AX8BpK. Or do I misunderstand you? https://codereview.chromium.org/14427006/diff/6001/chrome/browser/extensions/... File chrome/browser/extensions/api/declarative/deduping_factory_unittest.cc (right): https://codereview.chromium.org/14427006/diff/6001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative/deduping_factory_unittest.cc:53: scoped_refptr<const BaseClass> CreateFoo(const std::string& instance_type, nit: if |instance_type| is unused, it is a good practise to put it in /* */, as a hint to the reader. (Search for "/*radians*/" in the Google C++ style guide to see this recommended. :) ) https://codereview.chromium.org/14427006/diff/6001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative/deduping_factory_unittest.cc:58: static_cast<const base::DictionaryValue*>(value); nit: Both for human readability, and also error checking, I believe using GetAsDictionary is preferable: CHECK(value->GetAsDictionary(&dict));
Hi Vaclav, On 2013/04/23 15:47:23, vabr (Chromium) wrote: > Hi Dominic, > > Thanks, LGTM with comments. > > The changes between PS1 and PS2 look fine, although I'm not sure why you > introduced the instance_type string in the FactoryMethod. Do you have a > particular use-case in mind, or is it just for future? If the latter, then I > would recommend against it -- smaller arguments list make for faster execution, > and adding a parameter in the future should not be hard. This is for the immediate next CL so that we can use this class for both Actions and ConditionAttributes. It allows to share the same factory method for two different entries (think of requestHeaders and excludeRequestHeaders). It's going to be legendary. Best regards, Dominic > Cheers, > Vaclav > > https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... > File chrome/browser/extensions/api/declarative/deduping_factory.h (right): > > https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... > chrome/browser/extensions/api/declarative/deduping_factory.h:84: size_t > max_number_prototypes_; > On 2013/04/23 14:20:50, battre wrote: > > On 2013/04/23 12:22:12, vabr (Chromium) wrote: > > > This should be at least const, but I would even recommend to make it a > > template > > > argument. That way it becomes a compile-time constant, will take no space, > and > > > possibly allow more optimizations. > > > > I have made it const (even though it is pretty uncommon to have const members > in > > Chrome in my opinion). > > I had code to make it a template argument but removed it > > because the syntax was too cumbersome and I felt that the advantages do not > > outweigh the ugly syntax. > > I don't mean to be a pain in the ass, so feel free to ignore this comment. :) > > What exactly was cumbersome? It already is a template, so < and > are already > there, there would be one line less in the definition, and the max_number would > be just on another place in the instantiation, no? > > Strange syntax is unfortunately the burden of C++, and should not drive the > design decision. > A good reason against template parameters is that they generate too many copies > of the code, trading mostly speed for binary size. If we avoided templates for > syntax, hardly anybody would use them. :) > > https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... > File chrome/browser/extensions/api/declarative/deduping_factory_unittest.cc > (right): > > https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... > chrome/browser/extensions/api/declarative/deduping_factory_unittest.cc:22: > virtual Type GetType() const = 0; > On 2013/04/23 14:20:50, battre wrote: > > On 2013/04/23 12:30:43, vabr (Chromium) wrote: > > > On 2013/04/23 12:22:12, vabr (Chromium) wrote: > > > > If GetType() (or type()) is only used in Equals, it should be protected. > > > > > > And if GetType/type is protected, then so should be Type. > > > > If we protect GetType(), Foo::Equals cannot call it anymore. > > Are you sure? All protected parts of a base class are accessible to children. > See http://ideone.com/AX8BpK. Or do I misunderstand you? > > https://codereview.chromium.org/14427006/diff/6001/chrome/browser/extensions/... > File chrome/browser/extensions/api/declarative/deduping_factory_unittest.cc > (right): > > https://codereview.chromium.org/14427006/diff/6001/chrome/browser/extensions/... > chrome/browser/extensions/api/declarative/deduping_factory_unittest.cc:53: > scoped_refptr<const BaseClass> CreateFoo(const std::string& instance_type, > nit: if |instance_type| is unused, it is a good practise to put it in /* */, as > a hint to the reader. (Search for "/*radians*/" in the Google C++ style guide to > see this recommended. :) ) > > https://codereview.chromium.org/14427006/diff/6001/chrome/browser/extensions/... > chrome/browser/extensions/api/declarative/deduping_factory_unittest.cc:58: > static_cast<const base::DictionaryValue*>(value); > nit: Both for human readability, and also error checking, I believe using > GetAsDictionary is preferable: > > CHECK(value->GetAsDictionary(&dict));
Thanks for the review. https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/declarative/deduping_factory.h (right): https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/declarative/deduping_factory.h:84: size_t max_number_prototypes_; On 2013/04/23 15:47:23, vabr (Chromium) wrote: > On 2013/04/23 14:20:50, battre wrote: > > On 2013/04/23 12:22:12, vabr (Chromium) wrote: > > > This should be at least const, but I would even recommend to make it a > > template > > > argument. That way it becomes a compile-time constant, will take no space, > and > > > possibly allow more optimizations. > > > > I have made it const (even though it is pretty uncommon to have const members > in > > Chrome in my opinion). > > I had code to make it a template argument but removed it > > because the syntax was too cumbersome and I felt that the advantages do not > > outweigh the ugly syntax. > > I don't mean to be a pain in the ass, so feel free to ignore this comment. :) > > What exactly was cumbersome? It already is a template, so < and > are already > there, there would be one line less in the definition, and the max_number would > be just on another place in the instantiation, no? > > Strange syntax is unfortunately the burden of C++, and should not drive the > design decision. > A good reason against template parameters is that they generate too many copies > of the code, trading mostly speed for binary size. If we avoided templates for > syntax, hardly anybody would use them. :) The point I disliked was this syntax to access the enums. DedupingFactory<WebRequestConditionAttribute, 5>::IS_PARAMETERIZED We can typdef it to hide the parameter. I suggest to do that in a follow-up CL but frankly I think this is micro optimization that won't gain anything noticeable. https://codereview.chromium.org/14427006/diff/6001/chrome/browser/extensions/... File chrome/browser/extensions/api/declarative/deduping_factory_unittest.cc (right): https://codereview.chromium.org/14427006/diff/6001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative/deduping_factory_unittest.cc:53: scoped_refptr<const BaseClass> CreateFoo(const std::string& instance_type, On 2013/04/23 15:47:23, vabr (Chromium) wrote: > nit: if |instance_type| is unused, it is a good practise to put it in /* */, as > a hint to the reader. (Search for "/*radians*/" in the Google C++ style guide to > see this recommended. :) ) Done. Teeth-gnashingly. https://codereview.chromium.org/14427006/diff/6001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative/deduping_factory_unittest.cc:58: static_cast<const base::DictionaryValue*>(value); On 2013/04/23 15:47:23, vabr (Chromium) wrote: > nit: Both for human readability, and also error checking, I believe using > GetAsDictionary is preferable: > > CHECK(value->GetAsDictionary(&dict)); Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/battre@chromium.org/14427006/17001
Retried try job too often on linux_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
Hi Dominic, One more comment to fix the failing unit tests. LGTM. Vaclav https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/declarative/deduping_factory.h (right): https://codereview.chromium.org/14427006/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/declarative/deduping_factory.h:84: size_t max_number_prototypes_; On 2013/04/29 17:42:34, battre wrote: > On 2013/04/23 15:47:23, vabr (Chromium) wrote: > > On 2013/04/23 14:20:50, battre wrote: > > > On 2013/04/23 12:22:12, vabr (Chromium) wrote: > > > > This should be at least const, but I would even recommend to make it a > > > template > > > > argument. That way it becomes a compile-time constant, will take no space, > > and > > > > possibly allow more optimizations. > > > > > > I have made it const (even though it is pretty uncommon to have const > members > > in > > > Chrome in my opinion). > > > I had code to make it a template argument but removed it > > > because the syntax was too cumbersome and I felt that the advantages do not > > > outweigh the ugly syntax. > > > > I don't mean to be a pain in the ass, so feel free to ignore this comment. :) > > > > What exactly was cumbersome? It already is a template, so < and > are already > > there, there would be one line less in the definition, and the max_number > would > > be just on another place in the instantiation, no? > > > > Strange syntax is unfortunately the burden of C++, and should not drive the > > design decision. > > A good reason against template parameters is that they generate too many > copies > > of the code, trading mostly speed for binary size. If we avoided templates for > > syntax, hardly anybody would use them. :) > > The point I disliked was this syntax to access the enums. > > DedupingFactory<WebRequestConditionAttribute, 5>::IS_PARAMETERIZED > > We can typdef it to hide the parameter. > > I suggest to do that in a follow-up CL but frankly I think this is micro > optimization that won't gain anything noticeable. Sure, that's fine with me, no follow-up CL necessary. https://codereview.chromium.org/14427006/diff/17001/chrome/browser/extensions... File chrome/browser/extensions/api/declarative/deduping_factory_unittest.cc (right): https://codereview.chromium.org/14427006/diff/17001/chrome/browser/extensions... chrome/browser/extensions/api/declarative/deduping_factory_unittest.cc:94: bool bad_message; Please initialise to false (also on other places in this test). This is why the release unit tests failed. https://codereview.chromium.org/14427006/diff/17001/chrome/browser/extensions... chrome/browser/extensions/api/declarative/deduping_factory_unittest.cc:101: ASSERT_TRUE(c1); nit (very optional): Consider adding "<< error" on asserts like this, which could help diagnose a problem with failure in the future (would not have helped this time, though).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/battre@chromium.org/14427006/37001
Message was sent while issue was closed.
Change committed as 197361 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
