Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9581034:
Disable the transfer_buffer_ cache by forcing it to be empty; work around (Closed)
|
Created:
8 years, 9 months ago by nfullagar Modified:
8 years, 9 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, apatrick_chromium Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionDisable the transfer_buffer_ cache by forcing it to be empty; work around
for Bastion. Todo added to re-enable when underlying sync is solved.
BUG=http://code.google.com/p/chromium/issues/detail?id=116285
TEST=Bastion
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124840
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Messages
Total messages: 14 (0 generated)
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nfullagar@google.com/9581034/2001
Change committed as 124840
http://codereview.chromium.org/9581034/diff/2001/content/common/gpu/client/co... File content/common/gpu/client/command_buffer_proxy.cc (right): http://codereview.chromium.org/9581034/diff/2001/content/common/gpu/client/co... content/common/gpu/client/command_buffer_proxy.cc:316: transfer_buffers_[id] = buffer; How is the SharedMemory ever destroyed and unmapped now?
http://codereview.chromium.org/9581034/diff/2001/content/common/gpu/client/co... File content/common/gpu/client/command_buffer_proxy.cc (right): http://codereview.chromium.org/9581034/diff/2001/content/common/gpu/client/co... content/common/gpu/client/command_buffer_proxy.cc:316: transfer_buffers_[id] = buffer; On 2012/03/05 22:55:13, piman wrote: > How is the SharedMemory ever destroyed and unmapped now? Good point. Now the shared memory handles won't get closed and the transfer buffers will all leak.
On 2012/03/05 23:00:38, apatrick_chromium wrote: > http://codereview.chromium.org/9581034/diff/2001/content/common/gpu/client/co... > File content/common/gpu/client/command_buffer_proxy.cc (right): > > http://codereview.chromium.org/9581034/diff/2001/content/common/gpu/client/co... > content/common/gpu/client/command_buffer_proxy.cc:316: transfer_buffers_[id] = > buffer; > On 2012/03/05 22:55:13, piman wrote: > > How is the SharedMemory ever destroyed and unmapped now? > > Good point. Now the shared memory handles won't get closed and the transfer > buffers will all leak. That's the same issue I brought up this morning in conversation. It's either this or we can't run Bastion.
On 2012/03/05 23:07:36, nfullagar wrote: > On 2012/03/05 23:00:38, apatrick_chromium wrote: > > > http://codereview.chromium.org/9581034/diff/2001/content/common/gpu/client/co... > > File content/common/gpu/client/command_buffer_proxy.cc (right): > > > > > http://codereview.chromium.org/9581034/diff/2001/content/common/gpu/client/co... > > content/common/gpu/client/command_buffer_proxy.cc:316: transfer_buffers_[id] = > > buffer; > > On 2012/03/05 22:55:13, piman wrote: > > > How is the SharedMemory ever destroyed and unmapped now? > > > > Good point. Now the shared memory handles won't get closed and the transfer > > buffers will all leak. > > That's the same issue I brought up this morning in conversation. > > It's either this or we can't run Bastion. (In terms of the time remaining for m18)
On 2012/03/05 23:07:36, nfullagar wrote: > On 2012/03/05 23:00:38, apatrick_chromium wrote: > > > http://codereview.chromium.org/9581034/diff/2001/content/common/gpu/client/co... > > File content/common/gpu/client/command_buffer_proxy.cc (right): > > > > > http://codereview.chromium.org/9581034/diff/2001/content/common/gpu/client/co... > > content/common/gpu/client/command_buffer_proxy.cc:316: transfer_buffers_[id] = > > buffer; > > On 2012/03/05 22:55:13, piman wrote: > > > How is the SharedMemory ever destroyed and unmapped now? > > > > Good point. Now the shared memory handles won't get closed and the transfer > > buffers will all leak. > > That's the same issue I brought up this morning in conversation. > > It's either this or we can't run Bastion. I think this is unacceptable for everything else that needs accelerated compositing though.
well, here are the options - leave it as-is for m18; Bastion will crash & untrusted code has the capability to crash trusted code. - merge the patch, transfer buffer resizing leaks. How bad is this? - come up with something better, but we're just about out of time for m18. On Mon, Mar 5, 2012 at 3:08 PM, <piman@chromium.org> wrote: > On 2012/03/05 23:07:36, nfullagar wrote: > >> On 2012/03/05 23:00:38, apatrick_chromium wrote: >> > >> > > http://codereview.chromium.**org/9581034/diff/2001/content/** > common/gpu/client/command_**buffer_proxy.cc<http://codereview.chromium.org/9581034/diff/2001/content/common/gpu/client/command_buffer_proxy.cc> > >> > File content/common/gpu/client/**command_buffer_proxy.cc (right): >> > >> > >> > > http://codereview.chromium.**org/9581034/diff/2001/content/** > common/gpu/client/command_**buffer_proxy.cc#newcode316<http://codereview.chromium.org/9581034/diff/2001/content/common/gpu/client/command_buffer_proxy.cc#newcode316> > >> > content/common/gpu/client/**command_buffer_proxy.cc:316: >> transfer_buffers_[id] >> > = > >> > buffer; >> > On 2012/03/05 22:55:13, piman wrote: >> > > How is the SharedMemory ever destroyed and unmapped now? >> > >> > Good point. Now the shared memory handles won't get closed and the >> transfer >> > buffers will all leak. >> > > That's the same issue I brought up this morning in conversation. >> > > It's either this or we can't run Bastion. >> > > I think this is unacceptable for everything else that needs accelerated > compositing though. > > http://codereview.chromium.**org/9581034/<http://codereview.chromium.org/9581... >
On Mon, Mar 5, 2012 at 3:17 PM, Nicholas Fullagar <nfullagar@google.com>wrote: > well, here are the options > - leave it as-is for m18; Bastion will crash & untrusted code has the > capability to crash trusted code. > - merge the patch, transfer buffer resizing leaks. How bad is this? > Bad: we'll run out of memory, out of address space, and out of file descriptors in renderers. We'll crash in very bad and subtle ways. That's without counting the perf cost. - come up with something better, but we're just about out of time for m18. > Instead of applying a band aid, can we find the actual problem and fix that? Since it's a regression, can't we bisect? Antoine > > > On Mon, Mar 5, 2012 at 3:08 PM, <piman@chromium.org> wrote: > >> On 2012/03/05 23:07:36, nfullagar wrote: >> >>> On 2012/03/05 23:00:38, apatrick_chromium wrote: >>> > >>> >> >> http://codereview.chromium.**org/9581034/diff/2001/content/** >> common/gpu/client/command_**buffer_proxy.cc<http://codereview.chromium.org/9581034/diff/2001/content/common/gpu/client/command_buffer_proxy.cc> >> >>> > File content/common/gpu/client/**command_buffer_proxy.cc (right): >>> > >>> > >>> >> >> http://codereview.chromium.**org/9581034/diff/2001/content/** >> common/gpu/client/command_**buffer_proxy.cc#newcode316<http://codereview.chromium.org/9581034/diff/2001/content/common/gpu/client/command_buffer_proxy.cc#newcode316> >> >>> > content/common/gpu/client/**command_buffer_proxy.cc:316: >>> transfer_buffers_[id] >>> >> = >> >>> > buffer; >>> > On 2012/03/05 22:55:13, piman wrote: >>> > > How is the SharedMemory ever destroyed and unmapped now? >>> > >>> > Good point. Now the shared memory handles won't get closed and the >>> transfer >>> > buffers will all leak. >>> >> >> That's the same issue I brought up this morning in conversation. >>> >> >> It's either this or we can't run Bastion. >>> >> >> I think this is unacceptable for everything else that needs accelerated >> compositing though. >> >> http://codereview.chromium.**org/9581034/<http://codereview.chromium.org/9581... >> > >
Yes, this is a temporary. Within the time frame (Thurs / Fri) this is what we have. It was checked in Friday, and should be in a canary build. I don't like the solution as it is either. Did the canary die? On Mon, Mar 5, 2012 at 3:25 PM, Antoine Labour <piman@chromium.org> wrote: > > > On Mon, Mar 5, 2012 at 3:17 PM, Nicholas Fullagar <nfullagar@google.com>wrote: > >> well, here are the options >> - leave it as-is for m18; Bastion will crash & untrusted code has the >> capability to crash trusted code. >> - merge the patch, transfer buffer resizing leaks. How bad is this? >> > > Bad: we'll run out of memory, out of address space, and out of file > descriptors in renderers. We'll crash in very bad and subtle ways. > That's without counting the perf cost. > > - come up with something better, but we're just about out of time for m18. >> > > Instead of applying a band aid, can we find the actual problem and fix > that? Since it's a regression, can't we bisect? > > Antoine > > >> >> >> On Mon, Mar 5, 2012 at 3:08 PM, <piman@chromium.org> wrote: >> >>> On 2012/03/05 23:07:36, nfullagar wrote: >>> >>>> On 2012/03/05 23:00:38, apatrick_chromium wrote: >>>> > >>>> >>> >>> http://codereview.chromium.**org/9581034/diff/2001/content/** >>> common/gpu/client/command_**buffer_proxy.cc<http://codereview.chromium.org/9581034/diff/2001/content/common/gpu/client/command_buffer_proxy.cc> >>> >>>> > File content/common/gpu/client/**command_buffer_proxy.cc (right): >>>> > >>>> > >>>> >>> >>> http://codereview.chromium.**org/9581034/diff/2001/content/** >>> common/gpu/client/command_**buffer_proxy.cc#newcode316<http://codereview.chromium.org/9581034/diff/2001/content/common/gpu/client/command_buffer_proxy.cc#newcode316> >>> >>>> > content/common/gpu/client/**command_buffer_proxy.cc:316: >>>> transfer_buffers_[id] >>>> >>> = >>> >>>> > buffer; >>>> > On 2012/03/05 22:55:13, piman wrote: >>>> > > How is the SharedMemory ever destroyed and unmapped now? >>>> > >>>> > Good point. Now the shared memory handles won't get closed and the >>>> transfer >>>> > buffers will all leak. >>>> >>> >>> That's the same issue I brought up this morning in conversation. >>>> >>> >>> It's either this or we can't run Bastion. >>>> >>> >>> I think this is unacceptable for everything else that needs accelerated >>> compositing though. >>> >>> http://codereview.chromium.**org/9581034/<http://codereview.chromium.org/9581... >>> >> >> >
On Mon, Mar 5, 2012 at 3:36 PM, Nicholas Fullagar <nfullagar@google.com>wrote: > Yes, this is a temporary. Within the time frame (Thurs / Fri) this is > what we have. It was checked in Friday, and should be in a canary build. > I don't like the solution as it is either. Did the canary die? > I don't know that the windows canary exercises the GPU process as much as, say, Chrome OS does (because of Pepper Flash). > > On Mon, Mar 5, 2012 at 3:25 PM, Antoine Labour <piman@chromium.org> wrote: > >> >> >> On Mon, Mar 5, 2012 at 3:17 PM, Nicholas Fullagar <nfullagar@google.com>wrote: >> >>> well, here are the options >>> - leave it as-is for m18; Bastion will crash & untrusted code has the >>> capability to crash trusted code. >>> - merge the patch, transfer buffer resizing leaks. How bad is this? >>> >> >> Bad: we'll run out of memory, out of address space, and out of file >> descriptors in renderers. We'll crash in very bad and subtle ways. >> That's without counting the perf cost. >> >> - come up with something better, but we're just about out of time for m18. >>> >> >> Instead of applying a band aid, can we find the actual problem and fix >> that? Since it's a regression, can't we bisect? >> >> Antoine >> >> >>> >>> >>> On Mon, Mar 5, 2012 at 3:08 PM, <piman@chromium.org> wrote: >>> >>>> On 2012/03/05 23:07:36, nfullagar wrote: >>>> >>>>> On 2012/03/05 23:00:38, apatrick_chromium wrote: >>>>> > >>>>> >>>> >>>> http://codereview.chromium.**org/9581034/diff/2001/content/** >>>> common/gpu/client/command_**buffer_proxy.cc<http://codereview.chromium.org/9581034/diff/2001/content/common/gpu/client/command_buffer_proxy.cc> >>>> >>>>> > File content/common/gpu/client/**command_buffer_proxy.cc (right): >>>>> > >>>>> > >>>>> >>>> >>>> http://codereview.chromium.**org/9581034/diff/2001/content/** >>>> common/gpu/client/command_**buffer_proxy.cc#newcode316<http://codereview.chromium.org/9581034/diff/2001/content/common/gpu/client/command_buffer_proxy.cc#newcode316> >>>> >>>>> > content/common/gpu/client/**command_buffer_proxy.cc:316: >>>>> transfer_buffers_[id] >>>>> >>>> = >>>> >>>>> > buffer; >>>>> > On 2012/03/05 22:55:13, piman wrote: >>>>> > > How is the SharedMemory ever destroyed and unmapped now? >>>>> > >>>>> > Good point. Now the shared memory handles won't get closed and the >>>>> transfer >>>>> > buffers will all leak. >>>>> >>>> >>>> That's the same issue I brought up this morning in conversation. >>>>> >>>> >>>> It's either this or we can't run Bastion. >>>>> >>>> >>>> I think this is unacceptable for everything else that needs accelerated >>>> compositing though. >>>> >>>> http://codereview.chromium.**org/9581034/<http://codereview.chromium.org/9581... >>>> >>> >>> >> >
I checked checked canary. 19.0.1060.0 has a new memory leak. Just go to a page that triggers the accelerated compositor and keep refreshing it. It loses a few megs each time. On Mon, Mar 5, 2012 at 3:42 PM, Antoine Labour <piman@chromium.org> wrote: > > > On Mon, Mar 5, 2012 at 3:36 PM, Nicholas Fullagar <nfullagar@google.com>wrote: > >> Yes, this is a temporary. Within the time frame (Thurs / Fri) this is >> what we have. It was checked in Friday, and should be in a canary build. >> I don't like the solution as it is either. Did the canary die? >> > > I don't know that the windows canary exercises the GPU process as much as, > say, Chrome OS does (because of Pepper Flash). > > >> >> On Mon, Mar 5, 2012 at 3:25 PM, Antoine Labour <piman@chromium.org>wrote: >> >>> >>> >>> On Mon, Mar 5, 2012 at 3:17 PM, Nicholas Fullagar <nfullagar@google.com>wrote: >>> >>>> well, here are the options >>>> - leave it as-is for m18; Bastion will crash & untrusted code has the >>>> capability to crash trusted code. >>>> - merge the patch, transfer buffer resizing leaks. How bad is this? >>>> >>> >>> Bad: we'll run out of memory, out of address space, and out of file >>> descriptors in renderers. We'll crash in very bad and subtle ways. >>> That's without counting the perf cost. >>> >>> - come up with something better, but we're just about out of time for >>>> m18. >>>> >>> >>> Instead of applying a band aid, can we find the actual problem and fix >>> that? Since it's a regression, can't we bisect? >>> >>> Antoine >>> >>> >>>> >>>> >>>> On Mon, Mar 5, 2012 at 3:08 PM, <piman@chromium.org> wrote: >>>> >>>>> On 2012/03/05 23:07:36, nfullagar wrote: >>>>> >>>>>> On 2012/03/05 23:00:38, apatrick_chromium wrote: >>>>>> > >>>>>> >>>>> >>>>> http://codereview.chromium.**org/9581034/diff/2001/content/** >>>>> common/gpu/client/command_**buffer_proxy.cc<http://codereview.chromium.org/9581034/diff/2001/content/common/gpu/client/command_buffer_proxy.cc> >>>>> >>>>>> > File content/common/gpu/client/**command_buffer_proxy.cc (right): >>>>>> > >>>>>> > >>>>>> >>>>> >>>>> http://codereview.chromium.**org/9581034/diff/2001/content/** >>>>> common/gpu/client/command_**buffer_proxy.cc#newcode316<http://codereview.chromium.org/9581034/diff/2001/content/common/gpu/client/command_buffer_proxy.cc#newcode316> >>>>> >>>>>> > content/common/gpu/client/**command_buffer_proxy.cc:316: >>>>>> transfer_buffers_[id] >>>>>> >>>>> = >>>>> >>>>>> > buffer; >>>>>> > On 2012/03/05 22:55:13, piman wrote: >>>>>> > > How is the SharedMemory ever destroyed and unmapped now? >>>>>> > >>>>>> > Good point. Now the shared memory handles won't get closed and the >>>>>> transfer >>>>>> > buffers will all leak. >>>>>> >>>>> >>>>> That's the same issue I brought up this morning in conversation. >>>>>> >>>>> >>>>> It's either this or we can't run Bastion. >>>>>> >>>>> >>>>> I think this is unacceptable for everything else that needs accelerated >>>>> compositing though. >>>>> >>>>> http://codereview.chromium.**org/9581034/<http://codereview.chromium.org/9581... >>>>> >>>> >>>> >>> >> > |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
