Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
940633004:
Added disjoint context class which manages disjoints within gpu timing. (Closed)
|
Created:
5 years, 10 months ago by David Yen Modified:
5 years, 10 months ago CC:
chromium-reviews, Ken Russell (switch to Gerrit), piman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded disjoint context class which manages disjoints within gpu timing.
In preperation of supporting tracing within WebGL, it is necessary to
support multiple disjoint states within a single GL context. This way,
the disjoint state will be separate from the traces we do internally
and the disjoint state to a WebGL user.
R=vmiura@chromium.org
BUG=451211
Patch Set 1 #Patch Set 2 : Removed reference to check and reset timer errors #Patch Set 3 : Initialize disjoint value to proper value #
Total comments: 25
Patch Set 4 : Modified GPUTiming so it can be initialized multiple times, fixed some expectations #Patch Set 5 : Applied comments from dcastagna #Patch Set 6 : Added gpu_timing lifetime unit test, modified remove disjoint context function #Patch Set 7 : Added specific reason why DisjointContext needs to outlive parent #Patch Set 8 : Simplified DisjointContext to simply be an ID instead of an object #
Total comments: 4
Patch Set 9 : Added releases and DCHECK in gpu_timing #
Total comments: 11
Messages
Total messages: 21 (5 generated)
dcastagna@chromium.org changed reviewers: + dcastagna@chromium.org
https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... File gpu/command_buffer/service/gpu_timing.cc (right): https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gpu_timing.cc:66: : gpu_timing_(gpu_timing), Can gpu_timing be null? I'd add a DCHECK. https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gpu_timing.cc:98: for (DisjointContext* context_iter : disjoint_contexts_) { Do we really need to handle the case where GPUTiming gets delete before the DisjointContextes it owns? Can't we just assert that disjoint_contexts_.size() is 0 here if that should never happen? https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gpu_timing.cc:176: if (disjoint_value != 0) { I personally like being explicit with != 0, but in a previous review they suggested me to omit it. https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gpu_timing.cc:178: for (DisjointContext* context_iter : disjoint_contexts_) { nit: "context_iter" is not an iterator, I'd rename it just "context". https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gpu_timing.cc:186: bool item_found = false; Assuming we can use algorithms from stl in Chromium. Wouldn't this be clearer with the erase-removeif idiom? It'd be something like: disjoint_contexts_.erase(std::remove_if(... https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... File gpu/command_buffer/service/gpu_timing.h (right): https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gpu_timing.h:108: std::vector<DisjointContext*> disjoint_contexts_; How many disjoint contexts do you think there are going to be? https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... File gpu/command_buffer/service/gpu_tracer.cc (right): https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gpu_tracer.cc:138: gpu_timing_(gpu_timing), if gpu_timing can't be null, I'd add a DCHECK. https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... File gpu/command_buffer/service/gpu_tracer.h (right): https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gpu_tracer.h:53: explicit GPUTracer(gles2::GLES2Decoder* decoder, GPUTiming* gpu_timing); nit: remove explicit https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... File gpu/command_buffer/service/gpu_tracer_unittest.cc (right): https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gpu_tracer_unittest.cc:153: explicit GPUTracerTester(gles2::GLES2Decoder* decoder, GPUTiming* gpu_timing) nit: explicit here is not needed anymore.
Thanks for the quick review! https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... File gpu/command_buffer/service/gpu_timing.cc (right): https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gpu_timing.cc:66: : gpu_timing_(gpu_timing), On 2015/02/19 18:15:32, dcastagna wrote: > Can gpu_timing be null? > I'd add a DCHECK. Done. https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gpu_timing.cc:98: for (DisjointContext* context_iter : disjoint_contexts_) { On 2015/02/19 18:15:32, dcastagna wrote: > Do we really need to handle the case where GPUTiming gets delete before the > DisjointContextes it owns? > > Can't we just assert that disjoint_contexts_.size() is 0 here if that should > never happen? Unfortunately I think this is necessary because GPUTiming returns scoped references of DisjointContexts. The use case I am thinking of is you have a DisjointContext within your class next to your GPUTiming object (we will most likely have something like this in the gles2_cmd_decoder for the WebGL Traces). If we do not handle both cases we are at the mercy of the order the destructors are called. https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gpu_timing.cc:176: if (disjoint_value != 0) { On 2015/02/19 18:15:32, dcastagna wrote: > I personally like being explicit with != 0, but in a previous review they > suggested me to omit it. I just moved this code from the previous version line 107, I can omit it though. https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gpu_timing.cc:178: for (DisjointContext* context_iter : disjoint_contexts_) { On 2015/02/19 18:15:32, dcastagna wrote: > nit: "context_iter" is not an iterator, I'd rename it just "context". Done. https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gpu_timing.cc:186: bool item_found = false; On 2015/02/19 18:15:32, dcastagna wrote: > Assuming we can use algorithms from stl in Chromium. > Wouldn't this be clearer with the erase-removeif idiom? > > It'd be something like: > disjoint_contexts_.erase(std::remove_if(... I was trying to avoid shifting memory because this is a vector, but I would be willing to replace it if you think swapping to the end and then deleting is too complicated. https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... File gpu/command_buffer/service/gpu_timing.h (right): https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gpu_timing.h:108: std::vector<DisjointContext*> disjoint_contexts_; On 2015/02/19 18:15:32, dcastagna wrote: > How many disjoint contexts do you think there are going to be? Most likely only a few, but going from 2 to n does not really add any complexity IMHO. https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... File gpu/command_buffer/service/gpu_tracer.cc (right): https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gpu_tracer.cc:138: gpu_timing_(gpu_timing), On 2015/02/19 18:15:32, dcastagna wrote: > if gpu_timing can't be null, I'd add a DCHECK. Done. https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... File gpu/command_buffer/service/gpu_tracer.h (right): https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gpu_tracer.h:53: explicit GPUTracer(gles2::GLES2Decoder* decoder, GPUTiming* gpu_timing); On 2015/02/19 18:15:32, dcastagna wrote: > nit: remove explicit Done.
https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... File gpu/command_buffer/service/gpu_timing.cc (right): https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gpu_timing.cc:98: for (DisjointContext* context_iter : disjoint_contexts_) { On 2015/02/19 at 19:00:47, David Yen wrote: > On 2015/02/19 18:15:32, dcastagna wrote: > > Do we really need to handle the case where GPUTiming gets delete before the > > DisjointContextes it owns? > > > > Can't we just assert that disjoint_contexts_.size() is 0 here if that should > > never happen? > > Unfortunately I think this is necessary because GPUTiming returns scoped references of DisjointContexts. The use case I am thinking of is you have a DisjointContext within your class next to your GPUTiming object (we will most likely have something like this in the gles2_cmd_decoder for the WebGL Traces). If we do not handle both cases we are at the mercy of the order the destructors are called. Isn't the lifetime of those objects completely controlled by us? If GPUTiming gets deleted you end up with DisjointContexts that are still allocated but can't really do anything. If you feel strong about this, since you have a better idea of the potential use cases, I'd suggest at least to add a test that covers this code. https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gpu_timing.cc:176: if (disjoint_value != 0) { On 2015/02/19 at 19:00:47, David Yen wrote: > On 2015/02/19 18:15:32, dcastagna wrote: > > I personally like being explicit with != 0, but in a previous review they > > suggested me to omit it. > > I just moved this code from the previous version line 107, I can omit it though. It was just FYI, I didn't necessarily expect any action on this comment. https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gpu_timing.cc:186: bool item_found = false; On 2015/02/19 at 19:00:47, David Yen wrote: > On 2015/02/19 18:15:32, dcastagna wrote: > > Assuming we can use algorithms from stl in Chromium. > > Wouldn't this be clearer with the erase-removeif idiom? > > > > It'd be something like: > > disjoint_contexts_.erase(std::remove_if(... > > I was trying to avoid shifting memory because this is a vector, but I would be willing to replace it if you think swapping to the end and then deleting is too complicated. If you want to avoid shifting a few pointers, what about something like: auto found = std::find( disjoint_contexts_.begin(), disjoint_contexts_.end(), context); DCHECK_NE(found, disjoint_contexts_.end()); *found = disjoint_contexts_.back(); disjoint_contexts_.pop_back();
https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... File gpu/command_buffer/service/gpu_timing.cc (right): https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gpu_timing.cc:98: for (DisjointContext* context_iter : disjoint_contexts_) { On 2015/02/19 20:55:52, dcastagna wrote: > On 2015/02/19 at 19:00:47, David Yen wrote: > > On 2015/02/19 18:15:32, dcastagna wrote: > > > Do we really need to handle the case where GPUTiming gets delete before the > > > DisjointContextes it owns? > > > > > > Can't we just assert that disjoint_contexts_.size() is 0 here if that should > > > never happen? > > > > Unfortunately I think this is necessary because GPUTiming returns scoped > references of DisjointContexts. The use case I am thinking of is you have a > DisjointContext within your class next to your GPUTiming object (we will most > likely have something like this in the gles2_cmd_decoder for the WebGL Traces). > If we do not handle both cases we are at the mercy of the order the destructors > are called. > > Isn't the lifetime of those objects completely controlled by us? > If GPUTiming gets deleted you end up with DisjointContexts that are still > allocated but can't really do anything. > > If you feel strong about this, since you have a better idea of the potential use > cases, I'd suggest at least to add a test that covers this code. I've added a test for this. I'm not referring to a case where you would want to continue using the disjoint context after the GPUTiming has been destroyed. I'm simply talking about the order of the destruction between the 2 objects. Here is a simple example: { struct Test { scoped_refptr<DisjointContext> disjoint_context; GPUTiming gpu_timing; } test; test.disjoint_context = test.gpu_timing.CreateDisjointContext(); } Because of the destruction order (~GPUTiming(), then ~DisjointContext()), this will cause a crash without that code. Technically the lifetime of the objects is controlled by us (user of the code), but I can't enforce such a strange limitation - that if they are declared at the same stack level, the DisjointContext must be declared afterwards. https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gpu_timing.cc:186: bool item_found = false; On 2015/02/19 20:55:53, dcastagna wrote: > On 2015/02/19 at 19:00:47, David Yen wrote: > > On 2015/02/19 18:15:32, dcastagna wrote: > > > Assuming we can use algorithms from stl in Chromium. > > > Wouldn't this be clearer with the erase-removeif idiom? > > > > > > It'd be something like: > > > disjoint_contexts_.erase(std::remove_if(... > > > > I was trying to avoid shifting memory because this is a vector, but I would be > willing to replace it if you think swapping to the end and then deleting is too > complicated. > > If you want to avoid shifting a few pointers, what about something like: > > auto found = std::find( > disjoint_contexts_.begin(), disjoint_contexts_.end(), context); > DCHECK_NE(found, disjoint_contexts_.end()); > *found = disjoint_contexts_.back(); > disjoint_contexts_.pop_back(); Done.
LGTM https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... File gpu/command_buffer/service/gpu_timing.cc (right): https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gpu_timing.cc:71: if (gpu_timing_) { Should we make sure that gpu_timing_ is set here? I don't think anyone would want to use a DisjointContext after the gpu_timing_ has been reset, and this method would silently start to return always false. That can be more confusing than an assertion. https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gpu_timing.cc:98: for (DisjointContext* context_iter : disjoint_contexts_) { On 2015/02/19 at 22:48:03, David Yen wrote: > On 2015/02/19 20:55:52, dcastagna wrote: > > On 2015/02/19 at 19:00:47, David Yen wrote: > > > On 2015/02/19 18:15:32, dcastagna wrote: > > > > Do we really need to handle the case where GPUTiming gets delete before the > > > > DisjointContextes it owns? > > > > > > > > Can't we just assert that disjoint_contexts_.size() is 0 here if that should > > > > never happen? > > > > > > Unfortunately I think this is necessary because GPUTiming returns scoped > > references of DisjointContexts. The use case I am thinking of is you have a > > DisjointContext within your class next to your GPUTiming object (we will most > > likely have something like this in the gles2_cmd_decoder for the WebGL Traces). > > If we do not handle both cases we are at the mercy of the order the destructors > > are called. > > > > Isn't the lifetime of those objects completely controlled by us? > > If GPUTiming gets deleted you end up with DisjointContexts that are still > > allocated but can't really do anything. > > > > If you feel strong about this, since you have a better idea of the potential use > > cases, I'd suggest at least to add a test that covers this code. > > I've added a test for this. > > I'm not referring to a case where you would want to continue using the disjoint context after the GPUTiming has been destroyed. I'm simply talking about the order of the destruction between the 2 objects. Here is a simple example: > > { > struct Test { > scoped_refptr<DisjointContext> disjoint_context; > GPUTiming gpu_timing; > } test; > test.disjoint_context = test.gpu_timing.CreateDisjointContext(); > } > > Because of the destruction order (~GPUTiming(), then ~DisjointContext()), this will cause a crash without that code. > > Technically the lifetime of the objects is controlled by us (user of the code), but I can't enforce such a strange limitation - that if they are declared at the same stack level, the DisjointContext must be declared afterwards. Sorry if I insist on this, but I still stand with my original suggestion. Where do you think we're going to use those objects in that way? What you added to the tests seems a really particular use case. A simple DCHECK_EQ(disjoint_contexts_.size(), 0) in the destructor would make it clear not to use it in that way, it would allow you to reduce the code, it would introduce the nice invariant of having DisjointContext::gpu_timing_ always != NULL and you could avoid adding if(gpu_timing_) checks before dereferencing it.
https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... File gpu/command_buffer/service/gpu_timing.cc (right): https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gpu_timing.cc:98: for (DisjointContext* context_iter : disjoint_contexts_) { On 2015/02/20 01:38:18, dcastagna wrote: > On 2015/02/19 at 22:48:03, David Yen wrote: > > On 2015/02/19 20:55:52, dcastagna wrote: > > > On 2015/02/19 at 19:00:47, David Yen wrote: > > > > On 2015/02/19 18:15:32, dcastagna wrote: > > > > > Do we really need to handle the case where GPUTiming gets delete before > the > > > > > DisjointContextes it owns? > > > > > > > > > > Can't we just assert that disjoint_contexts_.size() is 0 here if that > should > > > > > never happen? > > > > > > > > Unfortunately I think this is necessary because GPUTiming returns scoped > > > references of DisjointContexts. The use case I am thinking of is you have a > > > DisjointContext within your class next to your GPUTiming object (we will > most > > > likely have something like this in the gles2_cmd_decoder for the WebGL > Traces). > > > If we do not handle both cases we are at the mercy of the order the > destructors > > > are called. > > > > > > Isn't the lifetime of those objects completely controlled by us? > > > If GPUTiming gets deleted you end up with DisjointContexts that are still > > > allocated but can't really do anything. > > > > > > If you feel strong about this, since you have a better idea of the potential > use > > > cases, I'd suggest at least to add a test that covers this code. > > > > I've added a test for this. > > > > I'm not referring to a case where you would want to continue using the > disjoint context after the GPUTiming has been destroyed. I'm simply talking > about the order of the destruction between the 2 objects. Here is a simple > example: > > > > { > > struct Test { > > scoped_refptr<DisjointContext> disjoint_context; > > GPUTiming gpu_timing; > > } test; > > test.disjoint_context = test.gpu_timing.CreateDisjointContext(); > > } > > > > Because of the destruction order (~GPUTiming(), then ~DisjointContext()), this > will cause a crash without that code. > > > > Technically the lifetime of the objects is controlled by us (user of the > code), but I can't enforce such a strange limitation - that if they are declared > at the same stack level, the DisjointContext must be declared afterwards. > > Sorry if I insist on this, but I still stand with my original suggestion. > Where do you think we're going to use those objects in that way? What you added > to the tests seems a really particular use case. A simple > DCHECK_EQ(disjoint_contexts_.size(), 0) in the destructor would make it clear > not to use it in that way, it would allow you to reduce the code, it would > introduce the nice invariant of having DisjointContext::gpu_timing_ always != > NULL and you could avoid adding if(gpu_timing_) checks before dereferencing it. > We are going to use those objects in that way in gles2_cmd_decoder.cc. Next to line 1886 where we declare GPUTiming gpu_timing_, we will have a scoped_refptr<DisjointContext> disjoint_context_ member which will be initialized somewhere after line 2450 (most likely right after). This disjoint context will handle disjoint commands called from the client into the command buffer, which are calls to DoGetParameter() with the GPU_DISJOINT_EXT argument.
New patchsets have been uploaded after l-g-t-m from dcastagna@chromium.org
On 2015/02/20 02:02:57, David Yen wrote: > https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... > File gpu/command_buffer/service/gpu_timing.cc (right): > > https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... > gpu/command_buffer/service/gpu_timing.cc:98: for (DisjointContext* context_iter > : disjoint_contexts_) { > On 2015/02/20 01:38:18, dcastagna wrote: > > On 2015/02/19 at 22:48:03, David Yen wrote: > > > On 2015/02/19 20:55:52, dcastagna wrote: > > > > On 2015/02/19 at 19:00:47, David Yen wrote: > > > > > On 2015/02/19 18:15:32, dcastagna wrote: > > > > > > Do we really need to handle the case where GPUTiming gets delete > before > > the > > > > > > DisjointContextes it owns? > > > > > > > > > > > > Can't we just assert that disjoint_contexts_.size() is 0 here if that > > should > > > > > > never happen? > > > > > > > > > > Unfortunately I think this is necessary because GPUTiming returns scoped > > > > references of DisjointContexts. The use case I am thinking of is you have > a > > > > DisjointContext within your class next to your GPUTiming object (we will > > most > > > > likely have something like this in the gles2_cmd_decoder for the WebGL > > Traces). > > > > If we do not handle both cases we are at the mercy of the order the > > destructors > > > > are called. > > > > > > > > Isn't the lifetime of those objects completely controlled by us? > > > > If GPUTiming gets deleted you end up with DisjointContexts that are still > > > > allocated but can't really do anything. > > > > > > > > If you feel strong about this, since you have a better idea of the > potential > > use > > > > cases, I'd suggest at least to add a test that covers this code. > > > > > > I've added a test for this. > > > > > > I'm not referring to a case where you would want to continue using the > > disjoint context after the GPUTiming has been destroyed. I'm simply talking > > about the order of the destruction between the 2 objects. Here is a simple > > example: > > > > > > { > > > struct Test { > > > scoped_refptr<DisjointContext> disjoint_context; > > > GPUTiming gpu_timing; > > > } test; > > > test.disjoint_context = test.gpu_timing.CreateDisjointContext(); > > > } > > > > > > Because of the destruction order (~GPUTiming(), then ~DisjointContext()), > this > > will cause a crash without that code. > > > > > > Technically the lifetime of the objects is controlled by us (user of the > > code), but I can't enforce such a strange limitation - that if they are > declared > > at the same stack level, the DisjointContext must be declared afterwards. > > > > Sorry if I insist on this, but I still stand with my original suggestion. > > Where do you think we're going to use those objects in that way? What you > added > > to the tests seems a really particular use case. A simple > > DCHECK_EQ(disjoint_contexts_.size(), 0) in the destructor would make it clear > > not to use it in that way, it would allow you to reduce the code, it would > > introduce the nice invariant of having DisjointContext::gpu_timing_ always != > > NULL and you could avoid adding if(gpu_timing_) checks before dereferencing > it. > > > > We are going to use those objects in that way in gles2_cmd_decoder.cc. Next to > line 1886 where we declare GPUTiming gpu_timing_, we will have a > scoped_refptr<DisjointContext> disjoint_context_ member which will be > initialized somewhere after line 2450 (most likely right after). This disjoint > context will handle disjoint commands called from the client into the command > buffer, which are calls to DoGetParameter() with the GPU_DISJOINT_EXT argument. Sorry for the big change, I decided to simplify the DisjointContext so this will have to be reviewed again. I've decided to just not used scoped pointers and replace it with an integer. This makes it so disjoint contexts must be explicitly deleted, but it also makes it so it is almost impossible to actually do it correctly (for example, it is not safe for gpu_tracer to deallocate the ID in the destructor because gpu_timing_ could be deleted by then. This was the point of the scoped pointers but perhaps it was too much trouble than it was worth). The worst that can happen is an unused context will just continue to be set when a disjoint happens, given that we will always only have a few this is probably ok. I've still added a "ReleaseID()" function for completeness sake, if someone is sure about their lifetime of the DisjointContext perhaps they could call it. Since it is not actually practical to use it I am open to removing it also to avoid pitfalls someone could fall into. Let me know if anyone has an opinion on that. PTAL.
On 2015/02/20 at 03:02:20, dyen wrote: > On 2015/02/20 02:02:57, David Yen wrote: > > https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... > > File gpu/command_buffer/service/gpu_timing.cc (right): > > > > https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... > > gpu/command_buffer/service/gpu_timing.cc:98: for (DisjointContext* context_iter > > : disjoint_contexts_) { > > On 2015/02/20 01:38:18, dcastagna wrote: > > > On 2015/02/19 at 22:48:03, David Yen wrote: > > > > On 2015/02/19 20:55:52, dcastagna wrote: > > > > > On 2015/02/19 at 19:00:47, David Yen wrote: > > > > > > On 2015/02/19 18:15:32, dcastagna wrote: > > > > > > > Do we really need to handle the case where GPUTiming gets delete > > before > > > the > > > > > > > DisjointContextes it owns? > > > > > > > > > > > > > > Can't we just assert that disjoint_contexts_.size() is 0 here if that > > > should > > > > > > > never happen? > > > > > > > > > > > > Unfortunately I think this is necessary because GPUTiming returns scoped > > > > > references of DisjointContexts. The use case I am thinking of is you have > > a > > > > > DisjointContext within your class next to your GPUTiming object (we will > > > most > > > > > likely have something like this in the gles2_cmd_decoder for the WebGL > > > Traces). > > > > > If we do not handle both cases we are at the mercy of the order the > > > destructors > > > > > are called. > > > > > > > > > > Isn't the lifetime of those objects completely controlled by us? > > > > > If GPUTiming gets deleted you end up with DisjointContexts that are still > > > > > allocated but can't really do anything. > > > > > > > > > > If you feel strong about this, since you have a better idea of the > > potential > > > use > > > > > cases, I'd suggest at least to add a test that covers this code. > > > > > > > > I've added a test for this. > > > > > > > > I'm not referring to a case where you would want to continue using the > > > disjoint context after the GPUTiming has been destroyed. I'm simply talking > > > about the order of the destruction between the 2 objects. Here is a simple > > > example: > > > > > > > > { > > > > struct Test { > > > > scoped_refptr<DisjointContext> disjoint_context; > > > > GPUTiming gpu_timing; > > > > } test; > > > > test.disjoint_context = test.gpu_timing.CreateDisjointContext(); > > > > } > > > > > > > > Because of the destruction order (~GPUTiming(), then ~DisjointContext()), > > this > > > will cause a crash without that code. > > > > > > > > Technically the lifetime of the objects is controlled by us (user of the > > > code), but I can't enforce such a strange limitation - that if they are > > declared > > > at the same stack level, the DisjointContext must be declared afterwards. > > > > > > Sorry if I insist on this, but I still stand with my original suggestion. > > > Where do you think we're going to use those objects in that way? What you > > added > > > to the tests seems a really particular use case. A simple > > > DCHECK_EQ(disjoint_contexts_.size(), 0) in the destructor would make it clear > > > not to use it in that way, it would allow you to reduce the code, it would > > > introduce the nice invariant of having DisjointContext::gpu_timing_ always != > > > NULL and you could avoid adding if(gpu_timing_) checks before dereferencing > > it. > > > > > > > We are going to use those objects in that way in gles2_cmd_decoder.cc. Next to > > line 1886 where we declare GPUTiming gpu_timing_, we will have a > > scoped_refptr<DisjointContext> disjoint_context_ member which will be > > initialized somewhere after line 2450 (most likely right after). This disjoint > > context will handle disjoint commands called from the client into the command > > buffer, which are calls to DoGetParameter() with the GPU_DISJOINT_EXT argument. > > Sorry for the big change, I decided to simplify the DisjointContext so this will have > to be reviewed again. > Sure, no problems. This also gives me a chance to try not LGTM. > I've decided to just not used scoped pointers and replace it with an integer. This > makes it so disjoint contexts must be explicitly deleted, but it also makes it so > it is almost impossible to actually do it correctly (for example, it is not safe for > gpu_tracer to deallocate the ID in the destructor because gpu_timing_ could be > deleted by then. This was the point of the scoped pointers but perhaps it was > too much trouble than it was worth). > Aren't members destructed in inverse order of the declaration? gpu_timing should already been destructed after gpu_tracer. If you don't like to rely on that, why don't you make gpu_timing_ a scoped pointer and you set it to nullptr after setting gpu_tracer to nullptr? > The worst that can happen is an unused context will just continue to be set when > a disjoint happens, given that we will always only have a few this is probably ok. > I don't like the idea to leave DisjointContexts around when no one is referring to those ids. > I've still added a "ReleaseID()" function for completeness sake, if someone is > sure about their lifetime of the DisjointContext perhaps they could call it. I think they should. And we should make sure that we gpu_timing is destroyed, the number of DisjointContextes is 0. > Since it is not actually practical to use it I am open to removing it also to > avoid pitfalls someone could fall into. Let me know if anyone has an opinion > on that. Could you elaborate on why you think this is not practical to use? > > PTAL.
On 2015/02/23 20:05:54, dcastagna wrote: > On 2015/02/20 at 03:02:20, dyen wrote: > > On 2015/02/20 02:02:57, David Yen wrote: > > > > https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... > > > File gpu/command_buffer/service/gpu_timing.cc (right): > > > > > > > https://codereview.chromium.org/940633004/diff/40001/gpu/command_buffer/servi... > > > gpu/command_buffer/service/gpu_timing.cc:98: for (DisjointContext* > context_iter > > > : disjoint_contexts_) { > > > On 2015/02/20 01:38:18, dcastagna wrote: > > > > On 2015/02/19 at 22:48:03, David Yen wrote: > > > > > On 2015/02/19 20:55:52, dcastagna wrote: > > > > > > On 2015/02/19 at 19:00:47, David Yen wrote: > > > > > > > On 2015/02/19 18:15:32, dcastagna wrote: > > > > > > > > Do we really need to handle the case where GPUTiming gets delete > > > before > > > > the > > > > > > > > DisjointContextes it owns? > > > > > > > > > > > > > > > > Can't we just assert that disjoint_contexts_.size() is 0 here if > that > > > > should > > > > > > > > never happen? > > > > > > > > > > > > > > Unfortunately I think this is necessary because GPUTiming returns > scoped > > > > > > references of DisjointContexts. The use case I am thinking of is you > have > > > a > > > > > > DisjointContext within your class next to your GPUTiming object (we > will > > > > most > > > > > > likely have something like this in the gles2_cmd_decoder for the WebGL > > > > Traces). > > > > > > If we do not handle both cases we are at the mercy of the order the > > > > destructors > > > > > > are called. > > > > > > > > > > > > Isn't the lifetime of those objects completely controlled by us? > > > > > > If GPUTiming gets deleted you end up with DisjointContexts that are > still > > > > > > allocated but can't really do anything. > > > > > > > > > > > > If you feel strong about this, since you have a better idea of the > > > potential > > > > use > > > > > > cases, I'd suggest at least to add a test that covers this code. > > > > > > > > > > I've added a test for this. > > > > > > > > > > I'm not referring to a case where you would want to continue using the > > > > disjoint context after the GPUTiming has been destroyed. I'm simply > talking > > > > about the order of the destruction between the 2 objects. Here is a simple > > > > example: > > > > > > > > > > { > > > > > struct Test { > > > > > scoped_refptr<DisjointContext> disjoint_context; > > > > > GPUTiming gpu_timing; > > > > > } test; > > > > > test.disjoint_context = test.gpu_timing.CreateDisjointContext(); > > > > > } > > > > > > > > > > Because of the destruction order (~GPUTiming(), then > ~DisjointContext()), > > > this > > > > will cause a crash without that code. > > > > > > > > > > Technically the lifetime of the objects is controlled by us (user of the > > > > code), but I can't enforce such a strange limitation - that if they are > > > declared > > > > at the same stack level, the DisjointContext must be declared afterwards. > > > > > > > > Sorry if I insist on this, but I still stand with my original suggestion. > > > > Where do you think we're going to use those objects in that way? What you > > > added > > > > to the tests seems a really particular use case. A simple > > > > DCHECK_EQ(disjoint_contexts_.size(), 0) in the destructor would make it > clear > > > > not to use it in that way, it would allow you to reduce the code, it would > > > > introduce the nice invariant of having DisjointContext::gpu_timing_ always > != > > > > NULL and you could avoid adding if(gpu_timing_) checks before > dereferencing > > > it. > > > > > > > > > > We are going to use those objects in that way in gles2_cmd_decoder.cc. Next > to > > > line 1886 where we declare GPUTiming gpu_timing_, we will have a > > > scoped_refptr<DisjointContext> disjoint_context_ member which will be > > > initialized somewhere after line 2450 (most likely right after). This > disjoint > > > context will handle disjoint commands called from the client into the > command > > > buffer, which are calls to DoGetParameter() with the GPU_DISJOINT_EXT > argument. > > > > Sorry for the big change, I decided to simplify the DisjointContext so this > will have > > to be reviewed again. > > > > Sure, no problems. > This also gives me a chance to try not LGTM. > > > I've decided to just not used scoped pointers and replace it with an integer. > This > > makes it so disjoint contexts must be explicitly deleted, but it also makes it > so > > it is almost impossible to actually do it correctly (for example, it is not > safe for > > gpu_tracer to deallocate the ID in the destructor because gpu_timing_ could be > > deleted by then. This was the point of the scoped pointers but perhaps it was > > too much trouble than it was worth). > > > > Aren't members destructed in inverse order of the declaration? gpu_timing should > already been destructed after gpu_tracer. > If you don't like to rely on that, why don't you make gpu_timing_ a scoped > pointer and you set it to nullptr after setting gpu_tracer to nullptr? > That's exactly what I did with the previous patch with DisjointContext and GPUTiming to solve this problem isn't it? The whole point of this patch 8 was to not use scoped_pointers which means we cannot clean up cleanly if variables were declared in the wrong order. > > The worst that can happen is an unused context will just continue to be set > when > > a disjoint happens, given that we will always only have a few this is probably > ok. > > > > I don't like the idea to leave DisjointContexts around when no one is referring > to those ids. > > > I've still added a "ReleaseID()" function for completeness sake, if someone is > > sure about their lifetime of the DisjointContext perhaps they could call it. > > I think they should. And we should make sure that we gpu_timing is destroyed, > the number of DisjointContextes is 0. > > > Since it is not actually practical to use it I am open to removing it also to > > avoid pitfalls someone could fall into. Let me know if anyone has an opinion > > on that. > > Could you elaborate on why you think this is not practical to use? I don't think it is good programming practice to have relationships so brittle that the order of variables declared breaks things. Crashing/DCHECKIng because someone refactors something and moves gpu_timing_ above gpu_tracer_ is unacceptable to me, it would just leave them scratching their head. If you think using a scoped pointer so objects can clean themselves up is better I can revert back to patch 7. I don't think there's a solution that makes everyone happy here.
dyen@chromium.org changed reviewers: + piman@chromium.org
+piman for review of gpu/perftests/texture_upload_perftest.cc
dcastagna@chromium.org changed reviewers: - piman@chromium.org
https://codereview.chromium.org/940633004/diff/140001/gpu/command_buffer/serv... File gpu/command_buffer/service/gpu_timing.cc (right): https://codereview.chromium.org/940633004/diff/140001/gpu/command_buffer/serv... gpu/command_buffer/service/gpu_timing.cc:117: disjoint_contexts_[disjoint_context_id].valid_ = false; Why do you want to reuse it instead of simply removing it from disjoint_contexts_? https://codereview.chromium.org/940633004/diff/140001/gpu/command_buffer/serv... gpu/command_buffer/service/gpu_timing.cc:123: bool GPUTiming::CheckAndResetTimerErrors(int disjoint_context_id) { This forces every user of GPUTiming to have a disjoint_context_id. If you look at https://code.google.com/p/chromium/codesearch#chromium/src/gpu/perftests/text... it's not using one. https://codereview.chromium.org/940633004/diff/140001/gpu/command_buffer/serv... gpu/command_buffer/service/gpu_timing.cc:125: disjoint_context_id >= 0 && Why do silently ignore invalid values? Isn't it better to dcheck that the id is an index in bound and it's valid? https://codereview.chromium.org/940633004/diff/140001/gpu/command_buffer/serv... File gpu/command_buffer/service/gpu_timing.h (right): https://codereview.chromium.org/940633004/diff/140001/gpu/command_buffer/serv... gpu/command_buffer/service/gpu_timing.h:93: bool valid_; Do we need this? Can't we just keep valid context ids in disjoint_contexts_? https://codereview.chromium.org/940633004/diff/160001/gpu/command_buffer/serv... File gpu/command_buffer/service/gpu_timing.cc (right): https://codereview.chromium.org/940633004/diff/160001/gpu/command_buffer/serv... gpu/command_buffer/service/gpu_timing.cc:67: Initialize(gl_context); Why do you initialize the object here? gl_context defaults to nullptr, so when you create the object with the default parameter you're basically just initializing the members of this object twice. I'd go with one of those two options: - Initialize is public and takes a context. The constructor doesn't take a gl context. - Initialize is private, and the object gets initialized when is create from the constructor that takes a context. https://codereview.chromium.org/940633004/diff/160001/gpu/command_buffer/serv... gpu/command_buffer/service/gpu_timing.cc:74: void GPUTiming::Initialize(gfx::GLContext* gl_context) { Who's suppose to call this function? You call it from the constructor, with gl_context set to null, and then from gpu_tracer code in intialize with a real gl_context. https://codereview.chromium.org/940633004/diff/160001/gpu/command_buffer/serv... gpu/command_buffer/service/gpu_timing.cc:102: for (int i = 0; i < num_contexts; ++i) { What about using std::find_if? https://codereview.chromium.org/940633004/diff/160001/gpu/command_buffer/serv... gpu/command_buffer/service/gpu_timing.cc:114: bool GPUTiming::ReleaseDisjointContextID(int disjoint_context_id) { Could you test this in your test? https://codereview.chromium.org/940633004/diff/160001/gpu/command_buffer/serv... gpu/command_buffer/service/gpu_timing.cc:125: int count = 0; std::count_if? https://codereview.chromium.org/940633004/diff/160001/gpu/command_buffer/serv... gpu/command_buffer/service/gpu_timing.cc:135: if (timer_type_ == kTimerTypeDisjoint && If the timer_type_ is disjoint, and the context_id is invalid this method is going to return false. We should probably make sure that the context_id is always valid. https://codereview.chromium.org/940633004/diff/160001/gpu/command_buffer/serv... File gpu/command_buffer/service/gpu_timing.h (right): https://codereview.chromium.org/940633004/diff/160001/gpu/command_buffer/serv... gpu/command_buffer/service/gpu_timing.h:56: GPUTiming(gfx::GLContext* gl_context = NULL); nit: explicit Why do you pass the context here? Are you passing an actual context to the constructor anywhere? nullptr is preferred according to https://chromium-cpp.appspot.com/ https://codereview.chromium.org/940633004/diff/160001/gpu/command_buffer/serv... gpu/command_buffer/service/gpu_timing.h:71: bool CheckAndResetTimerErrors(int disjoint_context_id); Can you leave the previous method as well, so that this api can be used without creating a disjoint_context_id. https://codereview.chromium.org/940633004/diff/160001/gpu/command_buffer/serv... gpu/command_buffer/service/gpu_timing.h:97: std::vector<DisjointContext> disjoint_contexts_; You never shrink this vector when a context is released. https://codereview.chromium.org/940633004/diff/160001/gpu/command_buffer/serv... File gpu/command_buffer/service/gpu_tracer.cc (right): https://codereview.chromium.org/940633004/diff/160001/gpu/command_buffer/serv... gpu/command_buffer/service/gpu_tracer.cc:148: gpu_timing_->ReleaseDisjointContextID(disjoint_context_id_); DCHECK the return value of this? disjoint_context_id_ should be valid. https://codereview.chromium.org/940633004/diff/160001/gpu/perftests/texture_u... File gpu/perftests/texture_upload_perftest.cc (left): https://codereview.chromium.org/940633004/diff/160001/gpu/perftests/texture_u... gpu/perftests/texture_upload_perftest.cc:128: gpu_timing_.CheckAndResetTimerErrors(); This is needed to invalidate the disjoint error that is going to be checked when crrev.com/944073005 lands.
On 2015/02/23 at 22:21:19, dcastagna wrote: > https://codereview.chromium.org/940633004/diff/140001/gpu/command_buffer/serv... > File gpu/command_buffer/service/gpu_timing.cc (right): > > https://codereview.chromium.org/940633004/diff/140001/gpu/command_buffer/serv... > gpu/command_buffer/service/gpu_timing.cc:117: disjoint_contexts_[disjoint_context_id].valid_ = false; > Why do you want to reuse it instead of simply removing it from disjoint_contexts_? > > https://codereview.chromium.org/940633004/diff/140001/gpu/command_buffer/serv... > gpu/command_buffer/service/gpu_timing.cc:123: bool GPUTiming::CheckAndResetTimerErrors(int disjoint_context_id) { > This forces every user of GPUTiming to have a disjoint_context_id. > If you look at https://code.google.com/p/chromium/codesearch#chromium/src/gpu/perftests/text... it's not using one. > > https://codereview.chromium.org/940633004/diff/140001/gpu/command_buffer/serv... > gpu/command_buffer/service/gpu_timing.cc:125: disjoint_context_id >= 0 && > Why do silently ignore invalid values? > Isn't it better to dcheck that the id is an index in bound and it's valid? > > https://codereview.chromium.org/940633004/diff/140001/gpu/command_buffer/serv... > File gpu/command_buffer/service/gpu_timing.h (right): > > https://codereview.chromium.org/940633004/diff/140001/gpu/command_buffer/serv... > gpu/command_buffer/service/gpu_timing.h:93: bool valid_; > Do we need this? Can't we just keep valid context ids in disjoint_contexts_? > > https://codereview.chromium.org/940633004/diff/160001/gpu/command_buffer/serv... > File gpu/command_buffer/service/gpu_timing.cc (right): > > https://codereview.chromium.org/940633004/diff/160001/gpu/command_buffer/serv... > gpu/command_buffer/service/gpu_timing.cc:67: Initialize(gl_context); > Why do you initialize the object here? > gl_context defaults to nullptr, so when you create the object with the default parameter you're basically just initializing the members of this object twice. > > I'd go with one of those two options: > - Initialize is public and takes a context. The constructor doesn't take a gl context. > - Initialize is private, and the object gets initialized when is create from the constructor that takes a context. > > https://codereview.chromium.org/940633004/diff/160001/gpu/command_buffer/serv... > gpu/command_buffer/service/gpu_timing.cc:74: void GPUTiming::Initialize(gfx::GLContext* gl_context) { > Who's suppose to call this function? You call it from the constructor, with gl_context set to null, and then from gpu_tracer code in intialize with a real gl_context. > > https://codereview.chromium.org/940633004/diff/160001/gpu/command_buffer/serv... > gpu/command_buffer/service/gpu_timing.cc:102: for (int i = 0; i < num_contexts; ++i) { > What about using std::find_if? > > https://codereview.chromium.org/940633004/diff/160001/gpu/command_buffer/serv... > gpu/command_buffer/service/gpu_timing.cc:114: bool GPUTiming::ReleaseDisjointContextID(int disjoint_context_id) { > Could you test this in your test? > > https://codereview.chromium.org/940633004/diff/160001/gpu/command_buffer/serv... > gpu/command_buffer/service/gpu_timing.cc:125: int count = 0; > std::count_if? > > https://codereview.chromium.org/940633004/diff/160001/gpu/command_buffer/serv... > gpu/command_buffer/service/gpu_timing.cc:135: if (timer_type_ == kTimerTypeDisjoint && > If the timer_type_ is disjoint, and the context_id is invalid this method is going to return false. We should probably make sure that the context_id is always valid. > > https://codereview.chromium.org/940633004/diff/160001/gpu/command_buffer/serv... > File gpu/command_buffer/service/gpu_timing.h (right): > > https://codereview.chromium.org/940633004/diff/160001/gpu/command_buffer/serv... > gpu/command_buffer/service/gpu_timing.h:56: GPUTiming(gfx::GLContext* gl_context = NULL); > nit: explicit > > Why do you pass the context here? Are you passing an actual context to the constructor anywhere? > > nullptr is preferred according to https://chromium-cpp.appspot.com/ > > https://codereview.chromium.org/940633004/diff/160001/gpu/command_buffer/serv... > gpu/command_buffer/service/gpu_timing.h:71: bool CheckAndResetTimerErrors(int disjoint_context_id); > Can you leave the previous method as well, so that this api can be used without creating a disjoint_context_id. > > https://codereview.chromium.org/940633004/diff/160001/gpu/command_buffer/serv... > gpu/command_buffer/service/gpu_timing.h:97: std::vector<DisjointContext> disjoint_contexts_; > You never shrink this vector when a context is released. > > https://codereview.chromium.org/940633004/diff/160001/gpu/command_buffer/serv... > File gpu/command_buffer/service/gpu_tracer.cc (right): > > https://codereview.chromium.org/940633004/diff/160001/gpu/command_buffer/serv... > gpu/command_buffer/service/gpu_tracer.cc:148: gpu_timing_->ReleaseDisjointContextID(disjoint_context_id_); > DCHECK the return value of this? disjoint_context_id_ should be valid. > > https://codereview.chromium.org/940633004/diff/160001/gpu/perftests/texture_u... > File gpu/perftests/texture_upload_perftest.cc (left): > > https://codereview.chromium.org/940633004/diff/160001/gpu/perftests/texture_u... > gpu/perftests/texture_upload_perftest.cc:128: gpu_timing_.CheckAndResetTimerErrors(); > This is needed to invalidate the disjoint error that is going to be checked when crrev.com/944073005 lands. Sorry, I published some drafts for patch 8 I didn't mean to publish, please ignore those.
Message was sent while issue was closed.
piman@chromium.org changed reviewers: + piman@chromium.org
Message was sent while issue was closed.
this review says "closed", do you need me to review it?
Message was sent while issue was closed.
On 2015/02/24 02:00:45, piman (Very slow to review) wrote: > this review says "closed", do you need me to review it? We had a discussion about handling virtual contexts, something we really overlooked here. The disjoint status needs to be virtualized, not only for GpuTracing, but also for multiple GLContextVirtual instances hosted on a real GLContext. I think David is refactoring things so it'll look like: GLContext -> one GpuTiming instance -> multiple GpuTimingClients.
Message was sent while issue was closed.
On Mon, Feb 23, 2015 at 6:17 PM, <vmiura@chromium.org> wrote: > On 2015/02/24 02:00:45, piman (Very slow to review) wrote: > >> this review says "closed", do you need me to review it? >> > > We had a discussion about handling virtual contexts, something we really > overlooked here. The disjoint status needs to be virtualized, not only for > GpuTracing, but also for multiple GLContextVirtual instances hosted on a > real > GLContext. > > I think David is refactoring things so it'll look like: > GLContext -> one GpuTiming instance -> multiple GpuTimingClients. > Ok, I'll wait for another CL or a ping on this one then. > > > https://codereview.chromium.org/940633004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
