Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
491783003:
cc: Resource list for headup display (Closed)
Descriptioncc: Resource list for headup display
Current HeadUpDisplayLayerImpl allocates one resource
and reuse it continuously. This patch is trying to
allocate two resources and swap one of them
which is available.
so HeadUpDisplayLayerImpl does not need free the resource
while the layer is alive.
Bug=None
Committed: https://crrev.com/695d3e36e0735a5848562a3a4bb50a9c081ab2bd
Cr-Commit-Position: refs/heads/master@{#292690}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 13
Patch Set 4 : the patch with ScopedPtrVector #
Total comments: 5
Patch Set 5 : remove unnecessary run in common case. #
Total comments: 5
Patch Set 6 : #
Total comments: 1
Patch Set 7 : #
Total comments: 2
Patch Set 8 : rename to ResourceSizeIsEqualTo #
Messages
Total messages: 24 (1 generated)
Could you review this patch?
Thanks for the patch! https://codereview.chromium.org/491783003/diff/40001/cc/layers/heads_up_displ... File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/491783003/diff/40001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.cc:90: if (!resource_provider->InUseByConsumer((*iter)->id())) how come this doesn't check content_bounds? https://codereview.chromium.org/491783003/diff/40001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.cc:109: ReleaseResource(iter); calling erase during iteration, this invalidates the iterator. shouldn't you store the ones you intend to delete, and then do a separate walk over the to_delete list to actually do that work? maybe a vector would be nice for this reason considering the size of this thing will be like.. 2 or 3. https://codereview.chromium.org/491783003/diff/40001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.cc:114: if (found_unused_resource) if you make Acquire move the resource it chooses to the back of the list, and you do the acquire step before doing the release step, then this method can just skip freeing the back() resource. https://codereview.chromium.org/491783003/diff/40001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.cc:123: if (resource->id()) why would we have a resource without an id in the list? https://codereview.chromium.org/491783003/diff/40001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.cc:124: resource->Free(); the destructor already calls Free, you just need to destroy the object. https://codereview.chromium.org/491783003/diff/40001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.cc:126: delete resource; manual memory management :( what if you put the ScopedResource in a scoped_ptr and make the list a ScopedPtrVector? This whole method could just go away, right? You'd just erase the element to destroy it properly. https://codereview.chromium.org/491783003/diff/40001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.cc:136: hud_resource_ = AcquireResource(resource_provider); how about just using the resources_.back() and move the resource you'll use to the back always, instead of adding another member variable to track this. https://codereview.chromium.org/491783003/diff/40001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.cc:225: ReleaseResource(iter); If you erase while iterating the same list, you're going to have a bad time. https://codereview.chromium.org/491783003/diff/40001/cc/layers/heads_up_displ... File cc/layers/heads_up_display_layer_impl.h (right): https://codereview.chromium.org/491783003/diff/40001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.h:125: typedef std::list<ScopedResource*> HUDResourceList; move down to L128 just above the variable?
thanks for reviewing the patch and all comments. I will file new patch soon. BTW, I found something unusual. RenderProcess's HeadsUpDisplayLayerImpl destructor is not called, when the process is destroyed. if it is an issue, I will check it too. https://codereview.chromium.org/491783003/diff/40001/cc/layers/heads_up_displ... File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/491783003/diff/40001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.cc:90: if (!resource_provider->InUseByConsumer((*iter)->id())) On 2014/08/25 14:51:21, danakj wrote: > how come this doesn't check content_bounds? I was trying to remove all resources which have different size. https://codereview.chromium.org/491783003/diff/40001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.cc:126: delete resource; On 2014/08/25 14:51:21, danakj wrote: > manual memory management :( > > what if you put the ScopedResource in a scoped_ptr and make the list a > ScopedPtrVector? This whole method could just go away, right? You'd just erase > the element to destroy it properly. thanks for comments. ScopedPtrVector, this is what I want to use. :) I will make new patch with ScopedPtrVector soon. https://codereview.chromium.org/491783003/diff/40001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.cc:136: hud_resource_ = AcquireResource(resource_provider); On 2014/08/25 14:51:21, danakj wrote: > how about just using the resources_.back() and move the resource you'll use to > the back always, instead of adding another member variable to track this. okay, I will apply it in next patch. thank you. https://codereview.chromium.org/491783003/diff/40001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.cc:225: ReleaseResource(iter); On 2014/08/25 14:51:21, danakj wrote: > If you erase while iterating the same list, you're going to have a bad time. I will remember it.
On Tue, Aug 26, 2014 at 7:16 AM, <jungjik.lee@samsung.com> wrote: > thanks for reviewing the patch and all comments. > I will file new patch soon. > > BTW, I found something unusual. > RenderProcess's HeadsUpDisplayLayerImpl destructor is not called, when the > process is destroyed. if it is an issue, I will check it too. When the renderer shutsdown, it usually takes a fast path where it just exits, not cleaning up and destroying things. It might be from that. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/08/26 14:12:15, danakj wrote: > On Tue, Aug 26, 2014 at 7:16 AM, <mailto:jungjik.lee@samsung.com> wrote: > > > thanks for reviewing the patch and all comments. > > I will file new patch soon. > > > > BTW, I found something unusual. > > RenderProcess's HeadsUpDisplayLayerImpl destructor is not called, when the > > process is destroyed. if it is an issue, I will check it too. > > > When the renderer shutsdown, it usually takes a fast path where it just > exits, not cleaning up and destroying things. It might be from that. Oh, I see. thank you for the explanation. I've uploaded new patch. please take a look.
Looks better thanks https://codereview.chromium.org/491783003/diff/60001/cc/layers/heads_up_displ... File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/491783003/diff/60001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.cc:109: to_be_kept.push_back(resources_.take(it)); i feel like the common case will be that we keep all the resources, which is also the worst case for this code. how about instead doing something like this, where in the common case where we delete nothing, we just iterate and done. size_t all_count = resources_.size(); size_t deleted_count = 0; for (size_t i = 0; i < all_count - deleted_count; ++i) { if (...want to delete...) { resources_.swap(i, end() - deleted_count - 1); ++deleted_count; } } resources_.erase(end() - deleted_count, end()); https://codereview.chromium.org/491783003/diff/60001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.cc:122: nit: drop whitespace https://codereview.chromium.org/491783003/diff/60001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.cc:124: nit: drop whitespace https://codereview.chromium.org/491783003/diff/60001/cc/layers/heads_up_displ... File cc/layers/heads_up_display_layer_impl.h (right): https://codereview.chromium.org/491783003/diff/60001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.h:126: nit: drop whitespace
https://codereview.chromium.org/491783003/diff/60001/cc/layers/heads_up_displ... File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/491783003/diff/60001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.cc:109: to_be_kept.push_back(resources_.take(it)); On 2014/08/27 13:25:01, danakj wrote: > i feel like the common case will be that we keep all the resources, which is > also the worst case for this code. > > how about instead doing something like this, where in the common case where we > delete nothing, we just iterate and done. > > size_t all_count = resources_.size(); > size_t deleted_count = 0; > for (size_t i = 0; i < all_count - deleted_count; ++i) { > if (...want to delete...) { > resources_.swap(i, end() - deleted_count - 1); > ++deleted_count; > } > } > resources_.erase(end() - deleted_count, end()); thanks for the kind explanation. however for two reasons, I could not apply your explanation. one is that erase of ScopedPtrVector supports only iterator type params. the other is that if in the case that begin() and end() - 1 are same and has different size with others, we could not remove them. so I made new patch with stl::partition + fix all nits. please take a look again. thank you in advance.
On 2014/08/28 15:55:38, JungJik wrote: > https://codereview.chromium.org/491783003/diff/60001/cc/layers/heads_up_displ... > File cc/layers/heads_up_display_layer_impl.cc (right): > > https://codereview.chromium.org/491783003/diff/60001/cc/layers/heads_up_displ... > cc/layers/heads_up_display_layer_impl.cc:109: > to_be_kept.push_back(resources_.take(it)); > On 2014/08/27 13:25:01, danakj wrote: > > i feel like the common case will be that we keep all the resources, which is > > also the worst case for this code. > > > > how about instead doing something like this, where in the common case where we > > delete nothing, we just iterate and done. > > > > size_t all_count = resources_.size(); > > size_t deleted_count = 0; > > for (size_t i = 0; i < all_count - deleted_count; ++i) { > > if (...want to delete...) { > > resources_.swap(i, end() - deleted_count - 1); > > ++deleted_count; > > } > > } > > resources_.erase(end() - deleted_count, end()); > > thanks for the kind explanation. > however for two reasons, I could not apply your explanation. Sorry I don't understand this, can you explain more? > one is that erase of ScopedPtrVector supports only iterator type params. end() and (end() - N) should both be iterators? > the other is that if in the case that begin() and end() - 1 are same and has > different size with others, we could not remove them. I am not sure what you mean here. if begin() and end()-1 are the same, there is 1 element in the list. what's wrong with that? > so I made new patch with stl::partition + fix all nits. > please take a look again. thank you in advance.
On 2014/08/28 16:44:24, danakj wrote: > On 2014/08/28 15:55:38, JungJik wrote: > > > https://codereview.chromium.org/491783003/diff/60001/cc/layers/heads_up_displ... > > File cc/layers/heads_up_display_layer_impl.cc (right): > > > > > https://codereview.chromium.org/491783003/diff/60001/cc/layers/heads_up_displ... > > cc/layers/heads_up_display_layer_impl.cc:109: > > to_be_kept.push_back(resources_.take(it)); > > On 2014/08/27 13:25:01, danakj wrote: > > > i feel like the common case will be that we keep all the resources, which is > > > also the worst case for this code. > > > > > > how about instead doing something like this, where in the common case where > we > > > delete nothing, we just iterate and done. > > > > > > size_t all_count = resources_.size(); > > > size_t deleted_count = 0; > > > for (size_t i = 0; i < all_count - deleted_count; ++i) { > > > if (...want to delete...) { > > > resources_.swap(i, end() - deleted_count - 1); > > > ++deleted_count; > > > } > > > } > > > resources_.erase(end() - deleted_count, end()); > > > > thanks for the kind explanation. > > however for two reasons, I could not apply your explanation. > > Sorry I don't understand this, can you explain more? > > > one is that erase of ScopedPtrVector supports only iterator type params. > > end() and (end() - N) should both be iterators? end() - N was okay, but in the case of end() - size_t(variable) - N, I got compiler error. I may have needed typecasting for size_t variable. > > > the other is that if in the case that begin() and end() - 1 are same and has > > different size with others, we could not remove them. > > I am not sure what you mean here. if begin() and end()-1 are the same, there is > 1 element in the list. what's wrong with that? sorry for my poor explanation. for example, we have { 1,2,3,4,5 } vector. if we want to remove odd, at the first iteration, 1 and 5 would be swapped. and deleted_count would be more than 1. so 5 would not be deleted. > > > so I made new patch with stl::partition + fix all nits. > > please take a look again. thank you in advance.
On Thu, Aug 28, 2014 at 1:17 PM, <jungjik.lee@samsung.com> wrote: > On 2014/08/28 16:44:24, danakj wrote: > >> On 2014/08/28 15:55:38, JungJik wrote: >> > >> > > https://codereview.chromium.org/491783003/diff/60001/cc/ > layers/heads_up_display_layer_impl.cc > >> > File cc/layers/heads_up_display_layer_impl.cc (right): >> > >> > >> > > https://codereview.chromium.org/491783003/diff/60001/cc/ > layers/heads_up_display_layer_impl.cc#newcode109 > >> > cc/layers/heads_up_display_layer_impl.cc:109: >> > to_be_kept.push_back(resources_.take(it)); >> > On 2014/08/27 13:25:01, danakj wrote: >> > > i feel like the common case will be that we keep all the resources, >> which >> > is > >> > > also the worst case for this code. >> > > >> > > how about instead doing something like this, where in the common case >> > where > >> we >> > > delete nothing, we just iterate and done. >> > > >> > > size_t all_count = resources_.size(); >> > > size_t deleted_count = 0; >> > > for (size_t i = 0; i < all_count - deleted_count; ++i) { >> > > if (...want to delete...) { >> > > resources_.swap(i, end() - deleted_count - 1); >> > > ++deleted_count; >> > > } >> > > } >> > > resources_.erase(end() - deleted_count, end()); >> > >> > thanks for the kind explanation. >> > however for two reasons, I could not apply your explanation. >> > > Sorry I don't understand this, can you explain more? >> > > > one is that erase of ScopedPtrVector supports only iterator type params. >> > > end() and (end() - N) should both be iterators? >> > > end() - N was okay, but in the case of end() - size_t(variable) - N, I got > compiler error. > I may have needed typecasting for size_t variable. What was the error? > > > > > the other is that if in the case that begin() and end() - 1 are same >> and has >> > different size with others, we could not remove them. >> > > I am not sure what you mean here. if begin() and end()-1 are the same, >> there >> > is > >> 1 element in the list. what's wrong with that? >> > > sorry for my poor explanation. for example, we have { 1,2,3,4,5 } vector. > if we > want to remove odd, > at the first iteration, 1 and 5 would be swapped. and deleted_count would > be > more than 1. > so 5 would not be deleted. Ok I'm confused still. {a,b,c,d,e}, deleted = 0. and you want to remove 1? so you swap. {e,b,c,d,a}, deleted = 1. then you erase(end - deleted, end) = erase(end - 1, end) which erases "a" from the array. > > > > > so I made new patch with stl::partition + fix all nits. >> > please take a look again. thank you in advance. >> > > > > https://codereview.chromium.org/491783003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/08/28 17:21:13, danakj wrote: > On Thu, Aug 28, 2014 at 1:17 PM, <mailto:jungjik.lee@samsung.com> wrote: > > > On 2014/08/28 16:44:24, danakj wrote: > > > >> On 2014/08/28 15:55:38, JungJik wrote: > >> > > >> > > > > https://codereview.chromium.org/491783003/diff/60001/cc/ > > layers/heads_up_display_layer_impl.cc > > > >> > File cc/layers/heads_up_display_layer_impl.cc (right): > >> > > >> > > >> > > > > https://codereview.chromium.org/491783003/diff/60001/cc/ > > layers/heads_up_display_layer_impl.cc#newcode109 > > > >> > cc/layers/heads_up_display_layer_impl.cc:109: > >> > to_be_kept.push_back(resources_.take(it)); > >> > On 2014/08/27 13:25:01, danakj wrote: > >> > > i feel like the common case will be that we keep all the resources, > >> which > >> > > is > > > >> > > also the worst case for this code. > >> > > > >> > > how about instead doing something like this, where in the common case > >> > > where > > > >> we > >> > > delete nothing, we just iterate and done. > >> > > > >> > > size_t all_count = resources_.size(); > >> > > size_t deleted_count = 0; > >> > > for (size_t i = 0; i < all_count - deleted_count; ++i) { > >> > > if (...want to delete...) { > >> > > resources_.swap(i, end() - deleted_count - 1); > >> > > ++deleted_count; > >> > > } > >> > > } > >> > > resources_.erase(end() - deleted_count, end()); > >> > > >> > thanks for the kind explanation. > >> > however for two reasons, I could not apply your explanation. > >> > > > > Sorry I don't understand this, can you explain more? > >> > > > > > one is that erase of ScopedPtrVector supports only iterator type params. > >> > > > > end() and (end() - N) should both be iterators? > >> > > > > end() - N was okay, but in the case of end() - size_t(variable) - N, I got > > compiler error. > > I may have needed typecasting for size_t variable. > > > What was the error? the error msg was like below. ../../cc/layers/heads_up_display_layer_impl.cc:127:58: error: invalid conversion from 'size_t {aka unsigned int}' to 'cc::ScopedPtrVector<cc::ScopedResource>::iterator {aka cc::ScopedResource**}' [-fpermissive] resources_.swap(i, resources_.end() - deleted_count - 1); ^ In file included from ../../cc/layers/layer_lists.h:12:0, from ../../cc/debug/debug_rect_history.h:11, from ../../cc/layers/heads_up_display_layer_impl.h:14, from ../../cc/layers/heads_up_display_layer_impl.cc:5: ../../cc/base/scoped_ptr_vector.h:154:8: error: initializing argument 1 of 'void cc::ScopedPtrVector<T>::swap(cc::ScopedPtrVector<T>::iterator, cc::ScopedPtrVector<T>::iterator) [with T = cc::ScopedResource; cc::ScopedPtrVector<T>::iterator = cc::ScopedResource**]' [-fpermissive] void swap(iterator a, iterator b) { > > > > I am not sure what you mean here. if begin() and end()-1 are the same, > >> there > >> > > is > > > >> 1 element in the list. what's wrong with that? > >> > > > > sorry for my poor explanation. for example, we have { 1,2,3,4,5 } vector. > > if we > > want to remove odd, > > at the first iteration, 1 and 5 would be swapped. and deleted_count would > > be > > more than 1. > > so 5 would not be deleted. > > > Ok I'm confused still. > > {a,b,c,d,e}, deleted = 0. and you want to remove 1? so you swap. > {e,b,c,d,a}, deleted = 1. > > then you erase(end - deleted, end) = erase(end - 1, end) which erases "a" > from the array. > I wrote simple python code for this. http://pastebin.com/raw.php?i=VqbCkbSk my intention was removing all unmatched size resources in the function. but if the unmatched size resources are placed in both begin and end, it wasn't working like my intention. thank you for reviewing this patch.
On Fri, Aug 29, 2014 at 11:01 AM, <jungjik.lee@samsung.com> wrote: > On 2014/08/28 17:21:13, danakj wrote: > > On Thu, Aug 28, 2014 at 1:17 PM, <mailto:jungjik.lee@samsung.com> wrote: >> > > > On 2014/08/28 16:44:24, danakj wrote: >> > >> >> On 2014/08/28 15:55:38, JungJik wrote: >> >> > >> >> >> > >> > https://codereview.chromium.org/491783003/diff/60001/cc/ >> > layers/heads_up_display_layer_impl.cc >> > >> >> > File cc/layers/heads_up_display_layer_impl.cc (right): >> >> > >> >> > >> >> >> > >> > https://codereview.chromium.org/491783003/diff/60001/cc/ >> > layers/heads_up_display_layer_impl.cc#newcode109 >> > >> >> > cc/layers/heads_up_display_layer_impl.cc:109: >> >> > to_be_kept.push_back(resources_.take(it)); >> >> > On 2014/08/27 13:25:01, danakj wrote: >> >> > > i feel like the common case will be that we keep all the resources, >> >> which >> >> >> > is >> > >> >> > > also the worst case for this code. >> >> > > >> >> > > how about instead doing something like this, where in the common >> case >> >> >> > where >> > >> >> we >> >> > > delete nothing, we just iterate and done. >> >> > > >> >> > > size_t all_count = resources_.size(); >> >> > > size_t deleted_count = 0; >> >> > > for (size_t i = 0; i < all_count - deleted_count; ++i) { >> >> > > if (...want to delete...) { >> >> > > resources_.swap(i, end() - deleted_count - 1); >> >> > > ++deleted_count; >> >> > > } >> >> > > } >> >> > > resources_.erase(end() - deleted_count, end()); >> >> > >> >> > thanks for the kind explanation. >> >> > however for two reasons, I could not apply your explanation. >> >> >> > >> > Sorry I don't understand this, can you explain more? >> >> >> > >> > > one is that erase of ScopedPtrVector supports only iterator type >> params. >> >> >> > >> > end() and (end() - N) should both be iterators? >> >> >> > >> > end() - N was okay, but in the case of end() - size_t(variable) - N, I >> got >> > compiler error. >> > I may have needed typecasting for size_t variable. >> > > > What was the error? >> > > the error msg was like below. > > ../../cc/layers/heads_up_display_layer_impl.cc:127:58: error: invalid > conversion > from 'size_t {aka unsigned int}' to > 'cc::ScopedPtrVector<cc::ScopedResource>::iterator {aka > cc::ScopedResource**}' > [-fpermissive] > resources_.swap(i, resources_.end() - deleted_count - 1); > ^ > In file included from ../../cc/layers/layer_lists.h:12:0, > from ../../cc/debug/debug_rect_history.h:11, > from ../../cc/layers/heads_up_display_layer_impl.h:14, > from ../../cc/layers/heads_up_display_layer_impl.cc:5: > ../../cc/base/scoped_ptr_vector.h:154:8: error: initializing argument 1 > of > 'void cc::ScopedPtrVector<T>::swap(cc::ScopedPtrVector<T>::iterator, > cc::ScopedPtrVector<T>::iterator) [with T = cc::ScopedResource; > cc::ScopedPtrVector<T>::iterator = cc::ScopedResource**]' [-fpermissive] > void swap(iterator a, iterator b) { > > > > >> > I am not sure what you mean here. if begin() and end()-1 are the same, >> >> there >> >> >> > is >> > >> >> 1 element in the list. what's wrong with that? >> >> >> > >> > sorry for my poor explanation. for example, we have { 1,2,3,4,5 } >> vector. >> > if we >> > want to remove odd, >> > at the first iteration, 1 and 5 would be swapped. and deleted_count >> would >> > be >> > more than 1. >> > so 5 would not be deleted. >> > > > Ok I'm confused still. >> > > {a,b,c,d,e}, deleted = 0. and you want to remove 1? so you swap. >> {e,b,c,d,a}, deleted = 1. >> > > then you erase(end - deleted, end) = erase(end - 1, end) which erases "a" >> from the array. >> > > > I wrote simple python code for this. http://pastebin.com/raw.php?i= > VqbCkbSk > my intention was removing all unmatched size resources in the function. > but if the unmatched size resources are placed in both begin and end, it > wasn't > working like my intention. > thank you for reviewing this patch. > resource_ = [1,2,3,4,5] all_count = len(resource_) deleted_count = 0; index = 0 while index < all_count - deleted_count: if resource_[index] % 2 == 1: # delete odds resource_[index], resource_[all_count - deleted_count - 1] = resource_[all_count - deleted_count - 1], resource_[index] deleted_count = deleted_count + 1 else: index = index + 1 print "expected result : [2, 4]" print "actual result :", resource_[:all_count-deleted_count] # give [4,2] which is correct To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/08/29 15:15:01, danakj wrote: > On Fri, Aug 29, 2014 at 11:01 AM, <mailto:jungjik.lee@samsung.com> wrote: > > > On 2014/08/28 17:21:13, danakj wrote: > > > > On Thu, Aug 28, 2014 at 1:17 PM, <mailto:jungjik.lee@samsung.com> wrote: > >> > > > > > On 2014/08/28 16:44:24, danakj wrote: > >> > > >> >> On 2014/08/28 15:55:38, JungJik wrote: > >> >> > > >> >> > >> > > >> > https://codereview.chromium.org/491783003/diff/60001/cc/ > >> > layers/heads_up_display_layer_impl.cc > >> > > >> >> > File cc/layers/heads_up_display_layer_impl.cc (right): > >> >> > > >> >> > > >> >> > >> > > >> > https://codereview.chromium.org/491783003/diff/60001/cc/ > >> > layers/heads_up_display_layer_impl.cc#newcode109 > >> > > >> >> > cc/layers/heads_up_display_layer_impl.cc:109: > >> >> > to_be_kept.push_back(resources_.take(it)); > >> >> > On 2014/08/27 13:25:01, danakj wrote: > >> >> > > i feel like the common case will be that we keep all the resources, > >> >> which > >> >> > >> > is > >> > > >> >> > > also the worst case for this code. > >> >> > > > >> >> > > how about instead doing something like this, where in the common > >> case > >> >> > >> > where > >> > > >> >> we > >> >> > > delete nothing, we just iterate and done. > >> >> > > > >> >> > > size_t all_count = resources_.size(); > >> >> > > size_t deleted_count = 0; > >> >> > > for (size_t i = 0; i < all_count - deleted_count; ++i) { > >> >> > > if (...want to delete...) { > >> >> > > resources_.swap(i, end() - deleted_count - 1); > >> >> > > ++deleted_count; > >> >> > > } > >> >> > > } > >> >> > > resources_.erase(end() - deleted_count, end()); > >> >> > > >> >> > thanks for the kind explanation. > >> >> > however for two reasons, I could not apply your explanation. > >> >> > >> > > >> > Sorry I don't understand this, can you explain more? > >> >> > >> > > >> > > one is that erase of ScopedPtrVector supports only iterator type > >> params. > >> >> > >> > > >> > end() and (end() - N) should both be iterators? > >> >> > >> > > >> > end() - N was okay, but in the case of end() - size_t(variable) - N, I > >> got > >> > compiler error. > >> > I may have needed typecasting for size_t variable. > >> > > > > > > What was the error? > >> > > > > the error msg was like below. > > > > ../../cc/layers/heads_up_display_layer_impl.cc:127:58: error: invalid > > conversion > > from 'size_t {aka unsigned int}' to > > 'cc::ScopedPtrVector<cc::ScopedResource>::iterator {aka > > cc::ScopedResource**}' > > [-fpermissive] > > resources_.swap(i, resources_.end() - deleted_count - 1); > > ^ > > In file included from ../../cc/layers/layer_lists.h:12:0, > > from ../../cc/debug/debug_rect_history.h:11, > > from ../../cc/layers/heads_up_display_layer_impl.h:14, > > from ../../cc/layers/heads_up_display_layer_impl.cc:5: > > ../../cc/base/scoped_ptr_vector.h:154:8: error: initializing argument 1 > > of > > 'void cc::ScopedPtrVector<T>::swap(cc::ScopedPtrVector<T>::iterator, > > cc::ScopedPtrVector<T>::iterator) [with T = cc::ScopedResource; > > cc::ScopedPtrVector<T>::iterator = cc::ScopedResource**]' [-fpermissive] > > void swap(iterator a, iterator b) { > > > > > > > > >> > I am not sure what you mean here. if begin() and end()-1 are the same, > >> >> there > >> >> > >> > is > >> > > >> >> 1 element in the list. what's wrong with that? > >> >> > >> > > >> > sorry for my poor explanation. for example, we have { 1,2,3,4,5 } > >> vector. > >> > if we > >> > want to remove odd, > >> > at the first iteration, 1 and 5 would be swapped. and deleted_count > >> would > >> > be > >> > more than 1. > >> > so 5 would not be deleted. > >> > > > > > > Ok I'm confused still. > >> > > > > {a,b,c,d,e}, deleted = 0. and you want to remove 1? so you swap. > >> {e,b,c,d,a}, deleted = 1. > >> > > > > then you erase(end - deleted, end) = erase(end - 1, end) which erases "a" > >> from the array. > >> > > > > > > I wrote simple python code for this. http://pastebin.com/raw.php?i= > > VqbCkbSk > > my intention was removing all unmatched size resources in the function. > > but if the unmatched size resources are placed in both begin and end, it > > wasn't > > working like my intention. > > thank you for reviewing this patch. > > > > resource_ = [1,2,3,4,5] > > all_count = len(resource_) > deleted_count = 0; > index = 0 > while index < all_count - deleted_count: > if resource_[index] % 2 == 1: # delete odds > resource_[index], resource_[all_count - deleted_count - 1] = > resource_[all_count - deleted_count - 1], resource_[index] > deleted_count = deleted_count + 1 > else: > index = index + 1 > > print "expected result : [2, 4]" > print "actual result :", resource_[:all_count-deleted_count] # give [4,2] > which is correct > oh now I see. thanks. so is the patch using stl::partition not acceptable?
https://codereview.chromium.org/491783003/diff/80001/cc/layers/heads_up_displ... File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/491783003/diff/80001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.cc:53: class CompareResourceSize { move this down above the function that uses it https://codereview.chromium.org/491783003/diff/80001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.cc:55: explicit CompareResourceSize(gfx::Size size_) : compare_size_(size_) {} const gfx::Size& https://codereview.chromium.org/491783003/diff/80001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.cc:58: return (resource->size() == compare_size_); drop the extra ()s https://codereview.chromium.org/491783003/diff/80001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.cc:121: it_erase = resources_.partition(CompareResourceSize(content_bounds())); if you wanna use partition, why do the for loop around it? Just call partition and done? https://codereview.chromium.org/491783003/diff/80001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.cc:125: if (it_erase != resources_.end()) drop the if, just erase.
Please take a look again :)
https://codereview.chromium.org/491783003/diff/100001/cc/layers/heads_up_disp... File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/491783003/diff/100001/cc/layers/heads_up_disp... cc/layers/heads_up_display_layer_impl.cc:116: ScopedPtrVector<ScopedResource>::iterator it_erase = resources_.end(); why set it to end() then immediately set it to partition()? Just set it to partition when you initialize it?
On 2014/08/29 17:38:25, danakj wrote: > https://codereview.chromium.org/491783003/diff/100001/cc/layers/heads_up_disp... > File cc/layers/heads_up_display_layer_impl.cc (right): > > https://codereview.chromium.org/491783003/diff/100001/cc/layers/heads_up_disp... > cc/layers/heads_up_display_layer_impl.cc:116: > ScopedPtrVector<ScopedResource>::iterator it_erase = resources_.end(); > why set it to end() then immediately set it to partition()? Just set it to > partition when you initialize it? sorry, I just found it and re-upload new patch.
LGTM w/ 1 name change https://codereview.chromium.org/491783003/diff/120001/cc/layers/heads_up_disp... File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/491783003/diff/120001/cc/layers/heads_up_disp... cc/layers/heads_up_display_layer_impl.cc:104: explicit CompareResourceSize(const gfx::Size& size_) : compare_size_(size_) {} rename this to ResourceSizeIsEqualTo compare is ambiguous as to comparing == or != or < or what.
https://codereview.chromium.org/491783003/diff/120001/cc/layers/heads_up_disp... File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/491783003/diff/120001/cc/layers/heads_up_disp... cc/layers/heads_up_display_layer_impl.cc:104: explicit CompareResourceSize(const gfx::Size& size_) : compare_size_(size_) {} On 2014/08/29 17:47:17, danakj wrote: > rename this to ResourceSizeIsEqualTo > > compare is ambiguous as to comparing == or != or < or what. done. thanks for lgtm and all your helps. :)
The CQ bit was checked by jungjik.lee@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jungjik.lee@samsung.com/491783003/140001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as 3ae0e8598a8de76f8540a706c4f34f51789b2fdf
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/695d3e36e0735a5848562a3a4bb50a9c081ab2bd Cr-Commit-Position: refs/heads/master@{#292690} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
