Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1491703003:
[Courgette] Initial Implementation of LabelManager. (Closed)
Description[Courgette] Initial Implementation of LabelManager
This is part of the effort to reduce Courgette's peak memory. Main changes:
- Moving Label to image_utils.h, and change Label::count_ from int to int32.
- Adding utility class ConsecutiveRangeVisitor, with tests.
- Adding LabelManager, with tests.
The new code is not yet used in production.
Committed: https://crrev.com/7a2fea2555a8014c4bdcdbc1119b97f98e057248
Cr-Commit-Position: refs/heads/master@{#363688}
Patch Set 1 #
Total comments: 20
Patch Set 2 : Style fixes; use C++11 features where applicable. #
Total comments: 43
Patch Set 3 : Optimization and cleanups. #
Total comments: 7
Patch Set 4 : Fix sign issues in tests. #Patch Set 5 : Fix signed/unsigned issue; make Label::count_ int32. #
Total comments: 12
Patch Set 6 : More signed/unsigned fix. #
Total comments: 2
Patch Set 7 : Fix comments; clean up test. #
Total comments: 2
Patch Set 8 : Fix const variable names in test; apply 'git cl format', keeping most recommendations. #
Total comments: 1
Patch Set 9 : Small spacing nit. #Patch Set 10 : Sync. #
Messages
Total messages: 38 (15 generated)
Description was changed from ========== [Courgette] Initial Implementation of LabelManager This is part of the effort to reduce Courgette's peak memory. Main changes: - Moving Label to image_utils.h. - Adding utility class ConsecutiveRangeVisitor, with tests. - Adding LabelManager, with tests. The new code is not yet used in production. ========== to ========== [Courgette] Initial Implementation of LabelManager This is part of the effort to reduce Courgette's peak memory. Main changes: - Moving Label to image_utils.h. - Adding utility class ConsecutiveRangeVisitor, with tests. - Adding LabelManager, with tests. The new code is not yet used in production. ==========
huangs@chromium.org changed reviewers: + chrisha@chromium.org, wfh@chromium.org
PTAL. Thanks!
grt@chromium.org changed reviewers: + grt@chromium.org
initial comments https://codereview.chromium.org/1491703003/diff/1/courgette/consecutive_range... File courgette/consecutive_range_visitor.h (right): https://codereview.chromium.org/1491703003/diff/1/courgette/consecutive_range... courgette/consecutive_range_visitor.h:49: InputIterator tail_; please add lightweight comments https://codereview.chromium.org/1491703003/diff/1/courgette/consecutive_range... courgette/consecutive_range_visitor.h:52: }; DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/1491703003/diff/1/courgette/image_utils.h File courgette/image_utils.h (right): https://codereview.chromium.org/1491703003/diff/1/courgette/image_utils.h#new... courgette/image_utils.h:26: static const int kNoIndex = -1; nit: enum : int { kNoIndex = -1 }; as per https://groups.google.com/a/chromium.org/d/msg/chromium-dev/hDqJ4KBVqog/aaAfR... https://codereview.chromium.org/1491703003/diff/1/courgette/image_utils.h#new... courgette/image_utils.h:35: RVA rva_; // Address referred to by the label. use non-static member initializers here; e.g.: RVA rva_ = 0; int index_ = kNoIndex; int count_ = 0; then remove the no-arg ctor altogether and only initialize rva_ in the other ctor https://codereview.chromium.org/1491703003/diff/1/courgette/label_manager.cc File courgette/label_manager.cc (right): https://codereview.chromium.org/1491703003/diff/1/courgette/label_manager.cc#... courgette/label_manager.cc:50: typedef ConsecutiveRangeVisitor<std::vector<RVA>::iterator> CRV; nit: using CRV = ConsecutiveRangeVisitor<std::vector<RVA>::iterator>; https://codereview.chromium.org/1491703003/diff/1/courgette/label_manager.cc#... courgette/label_manager.cc:57: DCHECK(labels_.empty()); have you tried using a std::deque rather than a std::vector? i wonder if the properties of a deque would make this "count then reserve then fill" go away. the code would be a bit simpler. https://codereview.chromium.org/1491703003/diff/1/courgette/label_manager.cc#... courgette/label_manager.cc:66: Label marker; unused https://codereview.chromium.org/1491703003/diff/1/courgette/label_manager.cc#... courgette/label_manager.cc:73: auto it = std::remove_if(labels_.begin(), labels_.end(), LabelHasZeroCount); lambda: [](const Label& l) { return l.count_ == 0; } https://codereview.chromium.org/1491703003/diff/1/courgette/label_manager.cc#... courgette/label_manager.cc:76: // Not calling labels_.shrink_to_fit(), since this may cause reallocation. shrink_to_fit can't be used in any case since it hasn't been approved for use in Chromium. i think it's fine to use the normal: labels_.erase(std::remove_if(...), labels_.end()); and do away with the if(). you could leave a comment on the line above saying not to shrink if you think that's relevant. https://codereview.chromium.org/1491703003/diff/1/courgette/label_manager.cc#... courgette/label_manager.cc:83: CompareLabelByRva); this is a perfect use of a lambda: [](const Label& l1, const Label& l2) { return l1.rva_ < l2.rva_; }
Thanks! Updated, PTAL. https://codereview.chromium.org/1491703003/diff/1/courgette/consecutive_range... File courgette/consecutive_range_visitor.h (right): https://codereview.chromium.org/1491703003/diff/1/courgette/consecutive_range... courgette/consecutive_range_visitor.h:49: InputIterator tail_; On 2015/12/02 19:03:40, grt wrote: > please add lightweight comments Done. https://codereview.chromium.org/1491703003/diff/1/courgette/consecutive_range... courgette/consecutive_range_visitor.h:52: }; I omitted this since I figured a visitor should be light-weight. But yeah adding makes more sense. Added. https://codereview.chromium.org/1491703003/diff/1/courgette/image_utils.h File courgette/image_utils.h (right): https://codereview.chromium.org/1491703003/diff/1/courgette/image_utils.h#new... courgette/image_utils.h:26: static const int kNoIndex = -1; On 2015/12/02 19:03:40, grt wrote: > nit: > enum : int { kNoIndex = -1 }; > as per > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/hDqJ4KBVqog/aaAfR... Done. https://codereview.chromium.org/1491703003/diff/1/courgette/image_utils.h#new... courgette/image_utils.h:35: RVA rva_; // Address referred to by the label. On 2015/12/02 19:03:40, grt wrote: > use non-static member initializers here; e.g.: > RVA rva_ = 0; > int index_ = kNoIndex; > int count_ = 0; > then remove the no-arg ctor altogether and only initialize rva_ in the other > ctor Done. https://codereview.chromium.org/1491703003/diff/1/courgette/label_manager.cc File courgette/label_manager.cc (right): https://codereview.chromium.org/1491703003/diff/1/courgette/label_manager.cc#... courgette/label_manager.cc:50: typedef ConsecutiveRangeVisitor<std::vector<RVA>::iterator> CRV; On 2015/12/02 19:03:41, grt wrote: > nit: > using CRV = ConsecutiveRangeVisitor<std::vector<RVA>::iterator>; Done. https://codereview.chromium.org/1491703003/diff/1/courgette/label_manager.cc#... courgette/label_manager.cc:57: DCHECK(labels_.empty()); On 2015/12/02 19:03:40, grt wrote: > have you tried using a std::deque rather than a std::vector? i wonder if the > properties of a deque would make this "count then reserve then fill" go away. > the code would be a bit simpler. Per discussion, the std::vector implementation is more amenable to the possible transition to using NoThrowBuffer(). Not doing this yet though, to keep changes small. https://codereview.chromium.org/1491703003/diff/1/courgette/label_manager.cc#... courgette/label_manager.cc:66: Label marker; Removed. https://codereview.chromium.org/1491703003/diff/1/courgette/label_manager.cc#... courgette/label_manager.cc:73: auto it = std::remove_if(labels_.begin(), labels_.end(), LabelHasZeroCount); On 2015/12/02 19:03:40, grt wrote: > lambda: > [](const Label& l) { return l.count_ == 0; } Woot! Done. https://codereview.chromium.org/1491703003/diff/1/courgette/label_manager.cc#... courgette/label_manager.cc:76: // Not calling labels_.shrink_to_fit(), since this may cause reallocation. Done, making comment more generic. https://codereview.chromium.org/1491703003/diff/1/courgette/label_manager.cc#... courgette/label_manager.cc:83: CompareLabelByRva); On 2015/12/02 19:03:41, grt wrote: > this is a perfect use of a lambda: > [](const Label& l1, const Label& l2) { return l1.rva_ < l2.rva_; } Done.
https://codereview.chromium.org/1491703003/diff/20001/courgette/consecutive_r... File courgette/consecutive_range_visitor.h (right): https://codereview.chromium.org/1491703003/diff/20001/courgette/consecutive_r... courgette/consecutive_range_visitor.h:45: while (head_ != end_ && *head_ == *tail_) is this not equivalent to lines 43-47? if (head_ != end_) while (++head_ != end_ && *head_ == *tail_) {} https://codereview.chromium.org/1491703003/diff/20001/courgette/consecutive_r... File courgette/consecutive_range_visitor_unittest.cc (right): https://codereview.chromium.org/1491703003/diff/20001/courgette/consecutive_r... courgette/consecutive_range_visitor_unittest.cc:14: namespace { i prefer that tests not be in the unnamed namespace as per https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pKY2G4GHcu0/FA5Wn... https://codereview.chromium.org/1491703003/diff/20001/courgette/consecutive_r... courgette/consecutive_range_visitor_unittest.cc:44: char s[] = "elephant elephant"; const char s[] = ...; https://codereview.chromium.org/1491703003/diff/20001/courgette/consecutive_r... courgette/consecutive_range_visitor_unittest.cc:45: size_t len = ::strlen(s); this and #include <cstring> aren't needed https://codereview.chromium.org/1491703003/diff/20001/courgette/consecutive_r... courgette/consecutive_range_visitor_unittest.cc:46: ConsecutiveRangeVisitor<char*> vis(s, s + len); as per http://chromium-cpp.appspot.com/#library-whitelist: #include <iterator> ConsecutiveRangeVisitor<char*> vis(std::begin(s), std::end(s) - 1); https://codereview.chromium.org/1491703003/diff/20001/courgette/consecutive_r... courgette/consecutive_range_visitor_unittest.cc:47: for (size_t i = 0; i < len; ++i) { simpler to loop until the string terminator is found for (const char* scan = &s[0]; *scan; ++scan) { https://codereview.chromium.org/1491703003/diff/20001/courgette/image_utils.h File courgette/image_utils.h (right): https://codereview.chromium.org/1491703003/diff/20001/courgette/image_utils.h... courgette/image_utils.h:23: // TODO(sra): Make fields private and add setters and getters. i think you can remove this TODO. based on https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes, this should be a struct rather than a class. as such, you could change it to: struct Label { enum : int { kNoIndex = -1 }; ... RVA rva = 0; // Address referred to by the label. ... }; note the lack of "public:" and trailing underscores on the data member names. https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager.cc File courgette/label_manager.cc (right): https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager... courgette/label_manager.cc:62: [](const Label& l) { return l.count_ == 0; }), can you do the mark-n-remove in one pass like so: [count_threshold](const Label& l) { return l.count_ < count_threshold; } ? https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager.h File courgette/label_manager.h (right): https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager... courgette/label_manager.h:31: virtual const RVA& Get() const = 0; an RVA is a 32-bit int, so treat it as a value type: virtual RVA Get() const = 0; https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager... courgette/label_manager.h:38: omit this newline https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager... courgette/label_manager.h:44: void Read(RvaVisitor* rva_visitor); it's hard to review this without seeing the consumer of this function. is implementing RvaVisitor easier than changing this function to: // Initializes the set of labels with the RVAs in |rvas| (ownership // of which is taken by this function). void Initialize(scoped_ptr<std::vector<RVA>> rvas); and moving the "Write all values in |rva_visitor| to |rvas|." part of Read() into the caller? https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager... File courgette/label_manager_unittest.cc (right): https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager... courgette/label_manager_unittest.cc:44: std::map<RVA, size_t>* expected) { pass |expected| by const-ref https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager... courgette/label_manager_unittest.cc:46: for (auto it : *expected) { |it| is a bad name since it doesn't name an iterator. i prefer something like: for (const auto& rva_and_count : expected) { since it names a pair containing an rva and a count https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager... courgette/label_manager_unittest.cc:53: } // namespace https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager... courgette/label_manager_unittest.cc:55: std::vector<RVA> test_targets = { C++-11 uniform initialization is still banned. you'll have to do this the old-fashioned way (e.g., repeated push_back or make an RVA[] and initialize the vector with the start/end of the array). https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager... courgette/label_manager_unittest.cc:74: std::map<RVA, size_t> expected1 = { same comment about this initialization syntax https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager... courgette/label_manager_unittest.cc:103: uint32 kRva = 12; uint32 -> const RVA https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager... courgette/label_manager_unittest.cc:106: std::vector<RVA> test_targets(dup, RVA(kRva)); RVA(kRva) -> kRva here and below https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager... courgette/label_manager_unittest.cc:113: EXPECT_TRUE(label != nullptr); EXPECT_NE? https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager... courgette/label_manager_unittest.cc:117: for (size_t i = 0; i < 16; ++i) { RVA is a typedef to uint32, so can you just use RVA here in the loop?
Thanks! Updated, PTAL. https://codereview.chromium.org/1491703003/diff/20001/courgette/consecutive_r... File courgette/consecutive_range_visitor.h (right): https://codereview.chromium.org/1491703003/diff/20001/courgette/consecutive_r... courgette/consecutive_range_visitor.h:45: while (head_ != end_ && *head_ == *tail_) On 2015/12/03 15:19:08, grt wrote: > is this not equivalent to lines 43-47? > if (head_ != end_) > while (++head_ != end_ && *head_ == *tail_) {} Done. https://codereview.chromium.org/1491703003/diff/20001/courgette/consecutive_r... File courgette/consecutive_range_visitor_unittest.cc (right): https://codereview.chromium.org/1491703003/diff/20001/courgette/consecutive_r... courgette/consecutive_range_visitor_unittest.cc:14: namespace { On 2015/12/03 15:19:09, grt wrote: > i prefer that tests not be in the unnamed namespace as per > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pKY2G4GHcu0/FA5Wn... Done. https://codereview.chromium.org/1491703003/diff/20001/courgette/consecutive_r... courgette/consecutive_range_visitor_unittest.cc:44: char s[] = "elephant elephant"; On 2015/12/03 15:19:09, grt wrote: > const char s[] = ...; Done. https://codereview.chromium.org/1491703003/diff/20001/courgette/consecutive_r... courgette/consecutive_range_visitor_unittest.cc:45: size_t len = ::strlen(s); On 2015/12/03 15:19:09, grt wrote: > this and #include <cstring> aren't needed Done. https://codereview.chromium.org/1491703003/diff/20001/courgette/consecutive_r... courgette/consecutive_range_visitor_unittest.cc:46: ConsecutiveRangeVisitor<char*> vis(s, s + len); Done, but <iterator> is already included in the .h file of the feature being tested. So excluding it. https://codereview.chromium.org/1491703003/diff/20001/courgette/consecutive_r... courgette/consecutive_range_visitor_unittest.cc:47: for (size_t i = 0; i < len; ++i) { On 2015/12/03 15:19:08, grt wrote: > simpler to loop until the string terminator is found > for (const char* scan = &s[0]; *scan; ++scan) { Done. https://codereview.chromium.org/1491703003/diff/20001/courgette/image_utils.h File courgette/image_utils.h (right): https://codereview.chromium.org/1491703003/diff/20001/courgette/image_utils.h... courgette/image_utils.h:23: // TODO(sra): Make fields private and add setters and getters. I'd rather hold off the "_" suffix change in a mechanical-change-only follow-up, since this leads to lots of changes in adjustment_method.cc and adjustment_method_2.cc. Replaced with TODO for myself. https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager.cc File courgette/label_manager.cc (right): https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager... courgette/label_manager.cc:62: [](const Label& l) { return l.count_ == 0; }), Ah nice! Using temp variable for the lambda. https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager.h File courgette/label_manager.h (right): https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager... courgette/label_manager.h:31: virtual const RVA& Get() const = 0; On 2015/12/03 15:19:09, grt wrote: > an RVA is a 32-bit int, so treat it as a value type: > virtual RVA Get() const = 0; Done. https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager... courgette/label_manager.h:38: On 2015/12/03 15:19:09, grt wrote: > omit this newline Done. https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager... courgette/label_manager.h:44: void Read(RvaVisitor* rva_visitor); The plan is that RvaVisitor will be specialized over {different architectures} x {Abs32, Rel32}. If we just make this a pure RVA translator, we'd miss out some optimizations that can be done with knowledge that Abs32 / Rel32 are sorted by address (e.g., for Win32 PE, Abs32 target lookup can have a "current section" state, without needing to loop over sections). And since we'd have differentiated code for looping, we may as well pass a visitor (iterator? generator?), and let LabelManager worry about algorithm & temp variable details. https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager... File courgette/label_manager_unittest.cc (right): https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager... courgette/label_manager_unittest.cc:44: std::map<RVA, size_t>* expected) { On 2015/12/03 15:19:09, grt wrote: > pass |expected| by const-ref Done. https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager... courgette/label_manager_unittest.cc:46: for (auto it : *expected) { On 2015/12/03 15:19:09, grt wrote: > |it| is a bad name since it doesn't name an iterator. i prefer something like: > for (const auto& rva_and_count : expected) { > since it names a pair containing an rva and a count Done. https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager... courgette/label_manager_unittest.cc:53: On 2015/12/03 15:19:09, grt wrote: > } // namespace Done. https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager... courgette/label_manager_unittest.cc:55: std::vector<RVA> test_targets = { On 2015/12/03 15:19:09, grt wrote: > C++-11 uniform initialization is still banned. you'll have to do this the > old-fashioned way (e.g., repeated push_back or make an RVA[] and initialize the > vector with the start/end of the array). Done. https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager... courgette/label_manager_unittest.cc:74: std::map<RVA, size_t> expected1 = { On 2015/12/03 15:19:09, grt wrote: > same comment about this initialization syntax Done. https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager... courgette/label_manager_unittest.cc:103: uint32 kRva = 12; On 2015/12/03 15:19:09, grt wrote: > uint32 -> const RVA Done. https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager... courgette/label_manager_unittest.cc:106: std::vector<RVA> test_targets(dup, RVA(kRva)); On 2015/12/03 15:19:10, grt wrote: > RVA(kRva) -> kRva > here and below Done. https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager... courgette/label_manager_unittest.cc:113: EXPECT_TRUE(label != nullptr); On 2015/12/03 15:19:09, grt wrote: > EXPECT_NE? Done. https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager... courgette/label_manager_unittest.cc:117: for (size_t i = 0; i < 16; ++i) { Done.
Ping wfh@ for review, PTAL. Thanks!
This lgtm. I'd like to wait for an l-g-t-m from grt before you commit though. Thanks. https://codereview.chromium.org/1491703003/diff/40001/courgette/label_manager... File courgette/label_manager_unittest.cc (right): https://codereview.chromium.org/1491703003/diff/40001/courgette/label_manager... courgette/label_manager_unittest.cc:16: namespace { can you move this out of anon namespace too? https://codereview.chromium.org/1491703003/diff/40001/courgette/label_manager... courgette/label_manager_unittest.cc:80: {0x00000110, 1}, will clang complain about these not being 1U?
Yeah I'll wait for grt@'s comments. Doing dry run in the mean time. Thanks! https://codereview.chromium.org/1491703003/diff/40001/courgette/label_manager... File courgette/label_manager_unittest.cc (right): https://codereview.chromium.org/1491703003/diff/40001/courgette/label_manager... courgette/label_manager_unittest.cc:16: namespace { Done; moving just TestRvaVisitor. https://codereview.chromium.org/1491703003/diff/40001/courgette/label_manager... courgette/label_manager_unittest.cc:80: {0x00000110, 1}, Don't know, but fixing these for consistency. Also cleaned up other sign issues in the tests.
The CQ bit was checked by huangs@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1491703003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491703003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) android_clang_dbg_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) android_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_android on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_android_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Description was changed from ========== [Courgette] Initial Implementation of LabelManager This is part of the effort to reduce Courgette's peak memory. Main changes: - Moving Label to image_utils.h. - Adding utility class ConsecutiveRangeVisitor, with tests. - Adding LabelManager, with tests. The new code is not yet used in production. ========== to ========== [Courgette] Initial Implementation of LabelManager This is part of the effort to reduce Courgette's peak memory. Main changes: - Moving Label to image_utils.h, and change Label::count_ from int to int32. - Adding utility class ConsecutiveRangeVisitor, with tests. - Adding LabelManager, with tests. The new code is not yet used in production. ==========
The CQ bit was checked by huangs@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1491703003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491703003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1491703003/diff/20001/courgette/image_utils.h File courgette/image_utils.h (right): https://codereview.chromium.org/1491703003/diff/20001/courgette/image_utils.h... courgette/image_utils.h:23: // TODO(sra): Make fields private and add setters and getters. On 2015/12/03 19:29:06, huangs wrote: > I'd rather hold off the "_" suffix change in a mechanical-change-only follow-up, > since this leads to lots of changes in adjustment_method.cc and > adjustment_method_2.cc. Replaced with TODO for myself. Acknowledged. https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager.cc File courgette/label_manager.cc (right): https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager... courgette/label_manager.cc:62: [](const Label& l) { return l.count_ == 0; }), On 2015/12/03 19:29:06, huangs wrote: > Ah nice! Using temp variable for the lambda. i think keeping the lambda inline is closer to what is shown in the style guide (https://google.github.io/styleguide/cppguide.html#Lambda_expressions): labels_.erase(std::remove_if(labels_.begin(), labels_.end(), [count_threshold](const Label& label) { return label.count_ < count_threshold; })); or whatever fascimile to that is accepted by git cl format. https://codereview.chromium.org/1491703003/diff/40001/courgette/label_manager... File courgette/label_manager_unittest.cc (right): https://codereview.chromium.org/1491703003/diff/40001/courgette/label_manager... courgette/label_manager_unittest.cc:16: namespace { On 2015/12/04 00:20:29, huangs wrote: > Done; moving just TestRvaVisitor. will and i are giving you conflicting advice! :-) my personal rule is that TEST functions and the fixtures used for TEST_F and TEST_P should not be in an unnamed namespace, whereas all private helper classes and functions should be. https://codereview.chromium.org/1491703003/diff/80001/courgette/image_utils.h File courgette/image_utils.h (right): https://codereview.chromium.org/1491703003/diff/80001/courgette/image_utils.h... courgette/image_utils.h:35: int32 count_ = 0; note that all new code should #include <stdint.h> and use the standard int32_t rather than int32. since the old types are used everywhere else, i don't expect you to change them all, just bringing this to your attention in case you didn't know about it for use in future coding adventures. :-) https://codereview.chromium.org/1491703003/diff/80001/courgette/label_manager.h File courgette/label_manager.h (right): https://codereview.chromium.org/1491703003/diff/80001/courgette/label_manager... courgette/label_manager.h:30: // Returns the current RVA (by value). remove " (by value)" since it documents the obvious. https://codereview.chromium.org/1491703003/diff/80001/courgette/label_manager... File courgette/label_manager_unittest.cc (right): https://codereview.chromium.org/1491703003/diff/80001/courgette/label_manager... courgette/label_manager_unittest.cc:27: for (auto rva_and_count : expected) { const auto& rva_and_count https://codereview.chromium.org/1491703003/diff/80001/courgette/label_manager... courgette/label_manager_unittest.cc:49: RVA Get() const override{ return *it_; } nit: space after "override" https://codereview.chromium.org/1491703003/diff/80001/courgette/label_manager... courgette/label_manager_unittest.cc:54: const std::vector<RVA>& rva_list_; holding a ref to an external object is a bit dangerous. how about taking the begin/end pair of iterators in the ctor? https://codereview.chromium.org/1491703003/diff/80001/courgette/label_manager... courgette/label_manager_unittest.cc:59: const RVA test_targets_raw[] = { static const RVA kTestTargetsRaw[] https://codereview.chromium.org/1491703003/diff/100001/courgette/consecutive_... File courgette/consecutive_range_visitor.h (right): https://codereview.chromium.org/1491703003/diff/100001/courgette/consecutive_... courgette/consecutive_range_visitor.h:14: // Before using this, check whether std::unique() is a better fit. do you plan to resolve this issue before landing the cl?
https://codereview.chromium.org/1491703003/diff/40001/courgette/label_manager... File courgette/label_manager_unittest.cc (right): https://codereview.chromium.org/1491703003/diff/40001/courgette/label_manager... courgette/label_manager_unittest.cc:16: namespace { On 2015/12/04 19:33:41, grt wrote: > On 2015/12/04 00:20:29, huangs wrote: > > Done; moving just TestRvaVisitor. > > will and i are giving you conflicting advice! :-) my personal rule is that TEST > functions and the fixtures used for TEST_F and TEST_P should not be in an > unnamed namespace, whereas all private helper classes and functions should be. woops sorry - listen to grt!
Updated, PTAL. https://codereview.chromium.org/1491703003/diff/40001/courgette/label_manager... File courgette/label_manager_unittest.cc (right): https://codereview.chromium.org/1491703003/diff/40001/courgette/label_manager... courgette/label_manager_unittest.cc:16: namespace { K, moving back. https://codereview.chromium.org/1491703003/diff/80001/courgette/image_utils.h File courgette/image_utils.h (right): https://codereview.chromium.org/1491703003/diff/80001/courgette/image_utils.h... courgette/image_utils.h:35: int32 count_ = 0; Ah okay. Thanks for the info! https://codereview.chromium.org/1491703003/diff/80001/courgette/label_manager.h File courgette/label_manager.h (right): https://codereview.chromium.org/1491703003/diff/80001/courgette/label_manager... courgette/label_manager.h:30: // Returns the current RVA (by value). I figured this signals intent better, but for POD it's obvious. Removed. https://codereview.chromium.org/1491703003/diff/80001/courgette/label_manager... File courgette/label_manager_unittest.cc (right): https://codereview.chromium.org/1491703003/diff/80001/courgette/label_manager... courgette/label_manager_unittest.cc:27: for (auto rva_and_count : expected) { On 2015/12/04 19:33:41, grt wrote: > const auto& rva_and_count Done. https://codereview.chromium.org/1491703003/diff/80001/courgette/label_manager... courgette/label_manager_unittest.cc:49: RVA Get() const override{ return *it_; } On 2015/12/04 19:33:41, grt wrote: > nit: space after "override" Done. https://codereview.chromium.org/1491703003/diff/80001/courgette/label_manager... courgette/label_manager_unittest.cc:54: const std::vector<RVA>& rva_list_; On 2015/12/04 19:33:41, grt wrote: > holding a ref to an external object is a bit dangerous. how about taking the > begin/end pair of iterators in the ctor? Done. https://codereview.chromium.org/1491703003/diff/80001/courgette/label_manager... courgette/label_manager_unittest.cc:59: const RVA test_targets_raw[] = { On 2015/12/04 19:33:41, grt wrote: > static const RVA kTestTargetsRaw[] Done, also applied below. https://codereview.chromium.org/1491703003/diff/100001/courgette/consecutive_... File courgette/consecutive_range_visitor.h (right): https://codereview.chromium.org/1491703003/diff/100001/courgette/consecutive_... courgette/consecutive_range_visitor.h:14: // Before using this, check whether std::unique() is a better fit. Oh, the comment is for whoever might want to reuse this code. Rephrased.
lgtm w/ a nit https://codereview.chromium.org/1491703003/diff/120001/courgette/label_manage... File courgette/label_manager_unittest.cc (right): https://codereview.chromium.org/1491703003/diff/120001/courgette/label_manage... courgette/label_manager_unittest.cc:60: static const RVA test_targets_raw[] = { kTestTargetsRaw (https://google.github.io/styleguide/cppguide.html#Constant_Names) and below
Thanks. Updated and submitting! https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager.cc File courgette/label_manager.cc (right): https://codereview.chromium.org/1491703003/diff/20001/courgette/label_manager... courgette/label_manager.cc:62: [](const Label& l) { return l.count_ == 0; }), Oops missed this. Ran "git cl format", too. https://codereview.chromium.org/1491703003/diff/120001/courgette/label_manage... File courgette/label_manager_unittest.cc (right): https://codereview.chromium.org/1491703003/diff/120001/courgette/label_manage... courgette/label_manager_unittest.cc:60: static const RVA test_targets_raw[] = { On 2015/12/04 20:29:02, grt wrote: > kTestTargetsRaw > (https://google.github.io/styleguide/cppguide.html#Constant_Names) > > and below Done.
https://codereview.chromium.org/1491703003/diff/140001/courgette/consecutive_... File courgette/consecutive_range_visitor.h (right): https://codereview.chromium.org/1491703003/diff/140001/courgette/consecutive_... courgette/consecutive_range_visitor.h:45: } Oh yeww, I'm moving brace back.
The CQ bit was checked by huangs@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wfh@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/1491703003/#ps160001 (title: "Small spacing nit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1491703003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491703003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by huangs@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wfh@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/1491703003/#ps180001 (title: "Sync.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1491703003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491703003/180001
Message was sent while issue was closed.
Description was changed from ========== [Courgette] Initial Implementation of LabelManager This is part of the effort to reduce Courgette's peak memory. Main changes: - Moving Label to image_utils.h, and change Label::count_ from int to int32. - Adding utility class ConsecutiveRangeVisitor, with tests. - Adding LabelManager, with tests. The new code is not yet used in production. ========== to ========== [Courgette] Initial Implementation of LabelManager This is part of the effort to reduce Courgette's peak memory. Main changes: - Moving Label to image_utils.h, and change Label::count_ from int to int32. - Adding utility class ConsecutiveRangeVisitor, with tests. - Adding LabelManager, with tests. The new code is not yet used in production. ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [Courgette] Initial Implementation of LabelManager This is part of the effort to reduce Courgette's peak memory. Main changes: - Moving Label to image_utils.h, and change Label::count_ from int to int32. - Adding utility class ConsecutiveRangeVisitor, with tests. - Adding LabelManager, with tests. The new code is not yet used in production. ========== to ========== [Courgette] Initial Implementation of LabelManager This is part of the effort to reduce Courgette's peak memory. Main changes: - Moving Label to image_utils.h, and change Label::count_ from int to int32. - Adding utility class ConsecutiveRangeVisitor, with tests. - Adding LabelManager, with tests. The new code is not yet used in production. Committed: https://crrev.com/7a2fea2555a8014c4bdcdbc1119b97f98e057248 Cr-Commit-Position: refs/heads/master@{#363688} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/7a2fea2555a8014c4bdcdbc1119b97f98e057248 Cr-Commit-Position: refs/heads/master@{#363688} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
