[chromium-dev] Re: Need help in finding a performance problem...
Salut again, I finally got it all working well on Windows (at least as far as I can see), and the numbers seem to say that this optimization would compensate for the slow down that was introduced when the resizer corner was enabled... I still have some doubts about the validity of the numbers I get on my machine, so please try it out if you have ways to consistently get similar profiling numbers on your machine... Also, please look at the code to make sure I don't do anything foolish in there... You can find it here: http://codereview.chromium.org/108040. Note that this still only works on Windows... I will start doing the changes to make it also work on Mac and Linux this afternoon... Once I get my post-lunch latté :-) Enjoy! BYE MAD :-) On Mon, May 11, 2009 at 8:44 PM, Marc-Andre Decoste wrote: > Feel free to ping me agl if you need help on Linux. ;) >> > > Will do... thanks for the offer :-) > > BYE > MAD > > On Mon, May 11, 2009 at 8:42 PM, Evan Martin wrote: > >> >> On Mon, May 11, 2009 at 5:40 PM, Marc-Andre Decoste >> wrote: >> >> However, I think that means if you leave the TransportDIB as the union >> >> >> >> of all the dirty rects, everything should Just Work. So your change >> >> >> >> becomes sending one unioned dirty rect image of pixel data, some of >> >> >> >> which that may not have actually been drawn to, along with an array of >> >> >> >> "actually dirty" subrectangles within the image. >> > >> > This is exactly what I'm doing on Windows... The receiving side loops on >> the >> > dirty sub-rectangles and get their pixels from the single btimap being >> > sent... And this bitmap is "only" valid in the areas of the >> sub-rectangles >> > passed in a vector via IPC. >> > Are we saying the same thing here? >> >> Yes. >> >> > P.S.: I'm still validating that it is all OK on the Windows side, just >> fixed >> > what I hope was the last refresh bug with Google maps, and then, I'll >> update >> > the GTK and Coco version of the painting code... >> >> Feel free to ping me agl if you need help on Linux. ;) >> >> >> >> > --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Need help in finding a performance problem...
> > Feel free to ping me agl if you need help on Linux. ;) > Will do... thanks for the offer :-) BYE MAD On Mon, May 11, 2009 at 8:42 PM, Evan Martin wrote: > > On Mon, May 11, 2009 at 5:40 PM, Marc-Andre Decoste > wrote: > >> However, I think that means if you leave the TransportDIB as the union > >> > >> of all the dirty rects, everything should Just Work. So your change > >> > >> becomes sending one unioned dirty rect image of pixel data, some of > >> > >> which that may not have actually been drawn to, along with an array of > >> > >> "actually dirty" subrectangles within the image. > > > > This is exactly what I'm doing on Windows... The receiving side loops on > the > > dirty sub-rectangles and get their pixels from the single btimap being > > sent... And this bitmap is "only" valid in the areas of the > sub-rectangles > > passed in a vector via IPC. > > Are we saying the same thing here? > > Yes. > > > P.S.: I'm still validating that it is all OK on the Windows side, just > fixed > > what I hope was the last refresh bug with Google maps, and then, I'll > update > > the GTK and Coco version of the painting code... > > Feel free to ping me agl if you need help on Linux. ;) > > > > --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Need help in finding a performance problem...
Salut Darin, this why I changed my initial approach (which was to send an array of smaller bitmaps alongside the array of dirty rects), to use a single bitmap with only the dirty rects drawn on it... To overcome a weird intermittent bug I was getting with our current usage of StretchDIBits, I now use a TransportDIB that is as big as the whole view rect (as opposed to the union rect)... I think it is OK, since we use a memory pool for those and we will have done a whole view rect invalidation at least once before getting into smaller invalidation, so that should use even less memory, even though we use a bigger bitmap then needed, right? BYE MAD On Mon, May 11, 2009 at 6:26 PM, Darin Fisher wrote: > On Mon, May 11, 2009 at 3:16 PM, Evan Martin wrote: > > On Sat, May 9, 2009 at 10:41 AM, Darin Fisher > wrote: > >>> At a high level, you're using one TransportDIB per rectangle, but it > >>> should be one per message (with multiple rects worth of image data > >>> inside). You can't really use any benchmarking results while this is > >>> the case. > >> > >> I agree. We should only require a single TransportDIB. It is > conceptually > >> just an array of pixels, so you should be able to append to that array > with > >> all of the new pixels and keep track of the offsets for each sub-bitmap. > > > > On X, the TransportDIB becomes a single Pixmap which we then bitblt > > from via X API calls. So we can't treat it *exactly* as a big array > > of pixels with offsets -- if the Pixmap is 400px wide and we have a > > tiny 20px wide dirty resize corner, we still need to write those 20px > > lines with a 400px stride. > > > > However, I think that means if you leave the TransportDIB as the union > > of all the dirty rects, everything should Just Work. So your change > > becomes sending one unioned dirty rect image of pixel data, some of > > which that may not have actually been drawn to, along with an array of > > "actually dirty" subrectangles within the image. > > > > I see my main concern was about wasting memory since each shared > memory allocation has significant overhead, and the minimize > allocation size is quite large, so even if you don't need much memory, > you end up using a lot of memory. > > -Darin > --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Need help in finding a performance problem...
On Mon, May 11, 2009 at 5:40 PM, Marc-Andre Decoste wrote: >> However, I think that means if you leave the TransportDIB as the union >> >> of all the dirty rects, everything should Just Work. So your change >> >> becomes sending one unioned dirty rect image of pixel data, some of >> >> which that may not have actually been drawn to, along with an array of >> >> "actually dirty" subrectangles within the image. > > This is exactly what I'm doing on Windows... The receiving side loops on the > dirty sub-rectangles and get their pixels from the single btimap being > sent... And this bitmap is "only" valid in the areas of the sub-rectangles > passed in a vector via IPC. > Are we saying the same thing here? Yes. > P.S.: I'm still validating that it is all OK on the Windows side, just fixed > what I hope was the last refresh bug with Google maps, and then, I'll update > the GTK and Coco version of the painting code... Feel free to ping me agl if you need help on Linux. ;) --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Need help in finding a performance problem...
> > However, I think that means if you leave the TransportDIB as the union > of all the dirty rects, everything should Just Work. So your change > becomes sending one unioned dirty rect image of pixel data, some of > which that may not have actually been drawn to, along with an array of > "actually dirty" subrectangles within the image. > This is exactly what I'm doing on Windows... The receiving side loops on the dirty sub-rectangles and get their pixels from the single btimap being sent... And this bitmap is "only" valid in the areas of the sub-rectangles passed in a vector via IPC. Are we saying the same thing here? BYE MAD P.S.: I'm still validating that it is all OK on the Windows side, just fixed what I hope was the last refresh bug with Google maps, and then, I'll update the GTK and Coco version of the painting code... On Mon, May 11, 2009 at 6:16 PM, Evan Martin wrote: > On Sat, May 9, 2009 at 10:41 AM, Darin Fisher wrote: > >> At a high level, you're using one TransportDIB per rectangle, but it > >> should be one per message (with multiple rects worth of image data > >> inside). You can't really use any benchmarking results while this is > >> the case. > > > > I agree. We should only require a single TransportDIB. It is > conceptually > > just an array of pixels, so you should be able to append to that array > with > > all of the new pixels and keep track of the offsets for each sub-bitmap. > > On X, the TransportDIB becomes a single Pixmap which we then bitblt > from via X API calls. So we can't treat it *exactly* as a big array > of pixels with offsets -- if the Pixmap is 400px wide and we have a > tiny 20px wide dirty resize corner, we still need to write those 20px > lines with a 400px stride. > > However, I think that means if you leave the TransportDIB as the union > of all the dirty rects, everything should Just Work. So your change > becomes sending one unioned dirty rect image of pixel data, some of > which that may not have actually been drawn to, along with an array of > "actually dirty" subrectangles within the image. > --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Need help in finding a performance problem...
On Mon, May 11, 2009 at 3:16 PM, Evan Martin wrote: > On Sat, May 9, 2009 at 10:41 AM, Darin Fisher wrote: >>> At a high level, you're using one TransportDIB per rectangle, but it >>> should be one per message (with multiple rects worth of image data >>> inside). You can't really use any benchmarking results while this is >>> the case. >> >> I agree. We should only require a single TransportDIB. It is conceptually >> just an array of pixels, so you should be able to append to that array with >> all of the new pixels and keep track of the offsets for each sub-bitmap. > > On X, the TransportDIB becomes a single Pixmap which we then bitblt > from via X API calls. So we can't treat it *exactly* as a big array > of pixels with offsets -- if the Pixmap is 400px wide and we have a > tiny 20px wide dirty resize corner, we still need to write those 20px > lines with a 400px stride. > > However, I think that means if you leave the TransportDIB as the union > of all the dirty rects, everything should Just Work. So your change > becomes sending one unioned dirty rect image of pixel data, some of > which that may not have actually been drawn to, along with an array of > "actually dirty" subrectangles within the image. > I see my main concern was about wasting memory since each shared memory allocation has significant overhead, and the minimize allocation size is quite large, so even if you don't need much memory, you end up using a lot of memory. -Darin --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Need help in finding a performance problem...
On Sat, May 9, 2009 at 10:41 AM, Darin Fisher wrote: >> At a high level, you're using one TransportDIB per rectangle, but it >> should be one per message (with multiple rects worth of image data >> inside). You can't really use any benchmarking results while this is >> the case. > > I agree. We should only require a single TransportDIB. It is conceptually > just an array of pixels, so you should be able to append to that array with > all of the new pixels and keep track of the offsets for each sub-bitmap. On X, the TransportDIB becomes a single Pixmap which we then bitblt from via X API calls. So we can't treat it *exactly* as a big array of pixels with offsets -- if the Pixmap is 400px wide and we have a tiny 20px wide dirty resize corner, we still need to write those 20px lines with a 400px stride. However, I think that means if you leave the TransportDIB as the union of all the dirty rects, everything should Just Work. So your change becomes sending one unioned dirty rect image of pixel data, some of which that may not have actually been drawn to, along with an array of "actually dirty" subrectangles within the image. --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Need help in finding a performance problem...
On Tue, May 5, 2009 at 3:08 PM, Adam Langley wrote: > On Tue, May 5, 2009 at 3:00 PM, Marc-Andre Decoste > wrote: > > That would be awsome... > > I just uploaded the patch here: > > http://codereview.chromium.org/108040 > > At a high level, you're using one TransportDIB per rectangle, but it > should be one per message (with multiple rects worth of image data > inside). You can't really use any benchmarking results while this is > the case. > > I agree. We should only require a single TransportDIB. It is conceptually just an array of pixels, so you should be able to append to that array with all of the new pixels and keep track of the offsets for each sub-bitmap. I believe that it is a good idea to change the code to not do smallest bounding box (for reasons beyond just the issue of the resize corner), but from our previous conversations, I'm still surprised that it would explain the performance impact here. I haven't studied this entire email thread though since it is quite large. -Darin --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Need help in finding a performance problem...
Salut, I've been trying to use a single bitmap, but it is much more work than I expected... And I couldn't get it to work flawlessly yet either... I get some refresh problems once in a while that I can't figure out... I solved many little bumps on the way, but now it works in most cases, except more complex pages like Google maps, which are giving me a lot of trouble... Here's what I'm doing: - In RenderWidget::DoDeferredPaint(), I create a drawing canvas the size of the union of all invalidated rects (I also tried creating one the size of the whole view rect, just to see... and it didn't help). - I translate the canvas based on the origin of the union rect, and then call webwidget_->Paint() for each invalidated rect individually, properly setting the clip rect on the canvas. (When I try with a bitmap the size of the whole view rect, I skip the translation, of course.) - I send that single bitmap with its rect and the list of sub-rects to paint. - In RenderWidgethost::PaintBackingStoreRects(), I now pass a vector of rects to paint, and the rect of the whole bitmap to paint from. I added a loop to call BackingStoreManager::PrepareBackingStore() for each rect to paint. - The BackingStore::PaintRect() method now receives the rect for the bitmap (as well as the rect to paint, since they are not necessarily the same anymore). I then use this to specify the source origin point in the bitmap where to point from. I still want to investigate a few things before I clean up the current version I have of this code and upload it to rietveld so that people can look at it and give me hints as to where I may have goofed... But if you already have hints as to what could not be working in the approach I described above, please send them my way... If you want more details about the approach and/or the problems that I'm having, let me know, and I will send them to you... Thanks! BYE MAD P.S.: I'm actually wondering if I'm wasting my time here... I tried to to some profiling anyway, even though the code doesn't work flawlessly... And I have not seen any dramatic improvements... Though as I said before, I don't think I can trust the numbers I get on my machine... So I keep digging... On Tue, May 5, 2009 at 6:26 PM, Marc-Andre Decoste wrote: > OK, I'll make the change... Interestingly, this was my original idea, but I > misunderstood your initial suggestions thinking you preferred an array of > bitmaps too... > I'll send a notice once I uploaded a new patch... > > Thanks! > > BYE > MAD > > > On Tue, May 5, 2009 at 6:08 PM, Adam Langley wrote: > >> On Tue, May 5, 2009 at 3:00 PM, Marc-Andre Decoste >> wrote: >> > That would be awsome... >> > I just uploaded the patch here: >> > http://codereview.chromium.org/108040 >> >> At a high level, you're using one TransportDIB per rectangle, but it >> should be one per message (with multiple rects worth of image data >> inside). You can't really use any benchmarking results while this is >> the case. >> >> >> AGL >> > > --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Need help in finding a performance problem...
OK, I'll make the change... Interestingly, this was my original idea, but I misunderstood your initial suggestions thinking you preferred an array of bitmaps too... I'll send a notice once I uploaded a new patch... Thanks! BYE MAD On Tue, May 5, 2009 at 6:08 PM, Adam Langley wrote: > On Tue, May 5, 2009 at 3:00 PM, Marc-Andre Decoste > wrote: > > That would be awsome... > > I just uploaded the patch here: > > http://codereview.chromium.org/108040 > > At a high level, you're using one TransportDIB per rectangle, but it > should be one per message (with multiple rects worth of image data > inside). You can't really use any benchmarking results while this is > the case. > > > AGL > --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Need help in finding a performance problem...
On Tue, May 5, 2009 at 3:00 PM, Marc-Andre Decoste wrote: > That would be awsome... > I just uploaded the patch here: > http://codereview.chromium.org/108040 At a high level, you're using one TransportDIB per rectangle, but it should be one per message (with multiple rects worth of image data inside). You can't really use any benchmarking results while this is the case. AGL --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Need help in finding a performance problem...
That would be awsome... I just uploaded the patch here: http://codereview.chromium.org/108040 BYE MAD On Tue, May 5, 2009 at 5:14 PM, Mike Pinkerton wrote: > I was able to get very consistent results before with just TestShell > on Mac running the mozilla page-cycler test locally. Would using > TestShell instead of Chromium help, or do you want me to try your > patch on Mac? > > Let me know. > > On Tue, May 5, 2009 at 2:08 PM, Marc-Andre Decoste > wrote: > > Salut, > >I've been trying to locally collect performance data to confirm > whether > > this was a good enough improvement or not and I can't seem to get > consistent > > enough results on my machine to draw a conclusion... Some things look > > faster, and others look slower, but I sometimes get faster results with > the > > resize corner enabled, compare to disabled (which doesn't really make > > sense)... So I don't think I can rely on these numbers... > >So I decided that I will go through with the code review of the > changes, > > and if Adam and Darin are happy with it, I will commit and monitor the > > performance dashboard to see how it goes there... If it goes bad, I'll > > revert... And if it goes well... I'll scream my happiness as loud as I > can > > (and those who know me a bit, know that I CAN be pretty LOUD!!! ;-)... > > BYE > > MAD > > > > On Fri, May 1, 2009 at 1:00 PM, Adam Langley wrote: > >> > >> On Fri, May 1, 2009 at 9:06 AM, Marc-Andre Decoste > >> wrote: > >> > Salut Evan, > >> > thanks, I will do that... And the results seems better than I > >> > initially > >> > thought... > >> > >> If you get performance improvements, please do commit :) > >> > >> Evan is correct that Darin needs to check this over, but I'll happy > >> code review everything where I can. > >> > >> > >> AGL > > > > > > > > -- > Mike Pinkerton > Mac Weenie > pinker...@google.com > --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Need help in finding a performance problem...
I was able to get very consistent results before with just TestShell on Mac running the mozilla page-cycler test locally. Would using TestShell instead of Chromium help, or do you want me to try your patch on Mac? Let me know. On Tue, May 5, 2009 at 2:08 PM, Marc-Andre Decoste wrote: > Salut, >I've been trying to locally collect performance data to confirm whether > this was a good enough improvement or not and I can't seem to get consistent > enough results on my machine to draw a conclusion... Some things look > faster, and others look slower, but I sometimes get faster results with the > resize corner enabled, compare to disabled (which doesn't really make > sense)... So I don't think I can rely on these numbers... >So I decided that I will go through with the code review of the changes, > and if Adam and Darin are happy with it, I will commit and monitor the > performance dashboard to see how it goes there... If it goes bad, I'll > revert... And if it goes well... I'll scream my happiness as loud as I can > (and those who know me a bit, know that I CAN be pretty LOUD!!! ;-)... > BYE > MAD > > On Fri, May 1, 2009 at 1:00 PM, Adam Langley wrote: >> >> On Fri, May 1, 2009 at 9:06 AM, Marc-Andre Decoste >> wrote: >> > Salut Evan, >> > thanks, I will do that... And the results seems better than I >> > initially >> > thought... >> >> If you get performance improvements, please do commit :) >> >> Evan is correct that Darin needs to check this over, but I'll happy >> code review everything where I can. >> >> >> AGL > > -- Mike Pinkerton Mac Weenie pinker...@google.com --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Need help in finding a performance problem...
Salut, I've been trying to locally collect performance data to confirm whether this was a good enough improvement or not and I can't seem to get consistent enough results on my machine to draw a conclusion... Some things look faster, and others look slower, but I sometimes get faster results with the resize corner enabled, compare to disabled (which doesn't really make sense)... So I don't think I can rely on these numbers... So I decided that I will go through with the code review of the changes, and if Adam and Darin are happy with it, I will commit and monitor the performance dashboard to see how it goes there... If it goes bad, I'll revert... And if it goes well... I'll scream my happiness as loud as I can (and those who know me a bit, know that I CAN be pretty LOUD!!! ;-)... BYE MAD On Fri, May 1, 2009 at 1:00 PM, Adam Langley wrote: > On Fri, May 1, 2009 at 9:06 AM, Marc-Andre Decoste > wrote: > > Salut Evan, > > thanks, I will do that... And the results seems better than I initially > > thought... > > If you get performance improvements, please do commit :) > > Evan is correct that Darin needs to check this over, but I'll happy > code review everything where I can. > > > AGL > --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Need help in finding a performance problem...
On Fri, May 1, 2009 at 9:06 AM, Marc-Andre Decoste wrote: > Salut Evan, > thanks, I will do that... And the results seems better than I initially > thought... If you get performance improvements, please do commit :) Evan is correct that Darin needs to check this over, but I'll happy code review everything where I can. AGL --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Need help in finding a performance problem...
Salut Evan, thanks, I will do that... And the results seems better than I initially thought... Now I need a better way to validate the results, because I think the high level evaluation I did previously was misleading... Looking at the data a little closer, I seem to have reached a much better performance than I thought, and the scenarios of with and without the resize corner are much closer than I initially thought with these two changes... So I'm going to work a bit more on making sure I have my numbers straight, and then get the gurus to validate my fixes :-) Thanks again! BYE MAD On Fri, May 1, 2009 at 11:54 AM, Evan Martin wrote: > On Fri, May 1, 2009 at 8:36 AM, Marc-Andre Decoste > wrote: > >So these don't seem to be THE reason for this slow down... So I keep > > digging... But I wonder if it would be worth it or not to commit these > > changes anyway, since they do provide a small performance improvement (at > > least in the scenario I'm working with, would be interesting to compare > to > > other scenarios, I may try that later today)... > > If you have improved page load performance as measured by the page > cyclers, it would seem very reasonable to commit it. Though you > probably want to run it by Darin first as there are a lot of subtle > details to how painting works. > --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Need help in finding a performance problem...
On Fri, May 1, 2009 at 8:36 AM, Marc-Andre Decoste wrote: > So these don't seem to be THE reason for this slow down... So I keep > digging... But I wonder if it would be worth it or not to commit these > changes anyway, since they do provide a small performance improvement (at > least in the scenario I'm working with, would be interesting to compare to > other scenarios, I may try that later today)... If you have improved page load performance as measured by the page cyclers, it would seem very reasonable to commit it. Though you probably want to run it by Darin first as there are a lot of subtle details to how painting works. --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Need help in finding a performance problem...
Salut again, I got a bit further, but not quite there yet... The (0, 0, 17, 17) invalidation request came from the fact that WebKit invalidates the scroll bars when their enabled state is set, and it was done "before" the frame rect of the scroll bar is set... So I fixed that (by simply reversing the call order) and so we save a few extraneous invalidations less... But that didn't make much of a difference in the timing unfortunately. Then I tried to pass an array of bitmaps and rects (as opposed to a union of rects) to the paint rect IPC message, and it cut off 100ms of my 3.2 seconds scenario (so down to 3.1s), but it also cuts off some time off of the scenario without the resizer rect (which went from about 2.72s to roughly 2.68s)... So these don't seem to be THE reason for this slow down... So I keep digging... But I wonder if it would be worth it or not to commit these changes anyway, since they do provide a small performance improvement (at least in the scenario I'm working with, would be interesting to compare to other scenarios, I may try that later today)... Again, thanks for any hints if you have some... ;-) BYE MAD On Thu, Apr 30, 2009 at 2:50 PM, Marc-Andre Decoste wrote: > As an intermediate point of reference, looking at the calls > to RenderWidget::DidInvalidateRect() and tracing the cases where the new > rect doesn't intersect with the current paint_rect, I get the following > results: > Without the resizer rect: > (0, 0, 743, 633) not in (0, 0, 0, 0) > (362, 8, 1, 1) not in (0, 0, 0, 0) > (726, 0, 17, 609) not in (0, 0, 0, 0) > (0, 0, 17, 17) not in (0, 0, 0, 0) > > With the resizer rect: > (0, 0, 743, 633) not in (0, 0, 0, 0) > (362, 8, 1, 1) not in (0, 0, 0, 0) > (726, 0, 17, 592) not in (0, 0, 0, 0) > (726, 592, 17, 17) not in (0, 0, 0, 0) > (0, 0, 17, 17) not in (726, 592, 17, 17) > (21, 149, 12, 25) not in (0, 0, 0, 0) > >So we do get a few more, and the scariest is the second last one, where > we get the two extreme corners of the window... I'll try to see if this > could actually be a bug where the position of the invalidation is wrong and > the intent was to get it at (726, 592) but it wasn't offset properly... But > since we also have that same (0, 0, 17, 17) request in the no resizer rect > scenario, I doubt it... > >Otherwise, I'll try to compute the timings with a new paint message that > receives an array of rects instead of a union... > > BYE > MAD > > On Thu, Apr 30, 2009 at 1:59 PM, Marc-Andre Decoste wrote: > >> Salut Adam, >>this is a theory that I'm currently validating... And I will try to >> change the IPC message code to confirm that it will resolve the problem... >> So I guess you don't see any problem in this approach... So if I succeed, >> now I know who to ask for a code review :-) >> >> Thanks! >> >> BYE >> MAD >> >> >> On Thu, Apr 30, 2009 at 1:44 PM, Adam Langley wrote: >> >>> On Thu, Apr 30, 2009 at 7:51 AM, Marc-Andre Decoste >>> wrote: >>> >An alternative could be to send a bitmap the size of the union rect, >>> but >>> > only paint the individual rects in it, and extract them individually on >>> the >>> > other side of the IPC... But I wonder if it would be worth the added >>> > complexity and risk... Unless I missed something (which is most >>> probably the >>> > case :-)... >>> >>> Forgive me if I'm misunderstanding here. But is the problem that the area >>> of the >>> union rectangle is significantly greater than the areas of the actually >>> damaged >>> regions, thus we're painting too much? >>> >>> If that's the case, we could well change the PaintRect and ScrollRect >>> messages >>> to carry a vector of rects and have them arranged in sequence in the >>> TransportDIB. Since I'm currently to blame for much of the IPC painting >>> code, I >>> can do this if it'll be of benefit. >>> >>> >>> AGL >>> >> >> > --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Need help in finding a performance problem...
As an intermediate point of reference, looking at the calls to RenderWidget::DidInvalidateRect() and tracing the cases where the new rect doesn't intersect with the current paint_rect, I get the following results: Without the resizer rect: (0, 0, 743, 633) not in (0, 0, 0, 0) (362, 8, 1, 1) not in (0, 0, 0, 0) (726, 0, 17, 609) not in (0, 0, 0, 0) (0, 0, 17, 17) not in (0, 0, 0, 0) With the resizer rect: (0, 0, 743, 633) not in (0, 0, 0, 0) (362, 8, 1, 1) not in (0, 0, 0, 0) (726, 0, 17, 592) not in (0, 0, 0, 0) (726, 592, 17, 17) not in (0, 0, 0, 0) (0, 0, 17, 17) not in (726, 592, 17, 17) (21, 149, 12, 25) not in (0, 0, 0, 0) So we do get a few more, and the scariest is the second last one, where we get the two extreme corners of the window... I'll try to see if this could actually be a bug where the position of the invalidation is wrong and the intent was to get it at (726, 592) but it wasn't offset properly... But since we also have that same (0, 0, 17, 17) request in the no resizer rect scenario, I doubt it... Otherwise, I'll try to compute the timings with a new paint message that receives an array of rects instead of a union... BYE MAD On Thu, Apr 30, 2009 at 1:59 PM, Marc-Andre Decoste wrote: > Salut Adam, >this is a theory that I'm currently validating... And I will try to > change the IPC message code to confirm that it will resolve the problem... > So I guess you don't see any problem in this approach... So if I succeed, > now I know who to ask for a code review :-) > > Thanks! > > BYE > MAD > > > On Thu, Apr 30, 2009 at 1:44 PM, Adam Langley wrote: > >> On Thu, Apr 30, 2009 at 7:51 AM, Marc-Andre Decoste >> wrote: >> >An alternative could be to send a bitmap the size of the union rect, >> but >> > only paint the individual rects in it, and extract them individually on >> the >> > other side of the IPC... But I wonder if it would be worth the added >> > complexity and risk... Unless I missed something (which is most probably >> the >> > case :-)... >> >> Forgive me if I'm misunderstanding here. But is the problem that the area >> of the >> union rectangle is significantly greater than the areas of the actually >> damaged >> regions, thus we're painting too much? >> >> If that's the case, we could well change the PaintRect and ScrollRect >> messages >> to carry a vector of rects and have them arranged in sequence in the >> TransportDIB. Since I'm currently to blame for much of the IPC painting >> code, I >> can do this if it'll be of benefit. >> >> >> AGL >> > > --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Need help in finding a performance problem...
Salut Adam, this is a theory that I'm currently validating... And I will try to change the IPC message code to confirm that it will resolve the problem... So I guess you don't see any problem in this approach... So if I succeed, now I know who to ask for a code review :-) Thanks! BYE MAD On Thu, Apr 30, 2009 at 1:44 PM, Adam Langley wrote: > On Thu, Apr 30, 2009 at 7:51 AM, Marc-Andre Decoste > wrote: > >An alternative could be to send a bitmap the size of the union rect, > but > > only paint the individual rects in it, and extract them individually on > the > > other side of the IPC... But I wonder if it would be worth the added > > complexity and risk... Unless I missed something (which is most probably > the > > case :-)... > > Forgive me if I'm misunderstanding here. But is the problem that the area > of the > union rectangle is significantly greater than the areas of the actually > damaged > regions, thus we're painting too much? > > If that's the case, we could well change the PaintRect and ScrollRect > messages > to carry a vector of rects and have them arranged in sequence in the > TransportDIB. Since I'm currently to blame for much of the IPC painting > code, I > can do this if it'll be of benefit. > > > AGL > --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Need help in finding a performance problem...
Salut Aaron, no, this is not recent... This is something that was encountered a while back and put on a shelf because it wasn't critical... I'm now taking it off the shelf and trying to put an end to it :-) Thanks! BYE MAD On Thu, Apr 30, 2009 at 1:56 PM, Aaron Boodman wrote: > Was this a recent regression? I made some changes in that area > recently to support transparent webviews. I tried not to change > anything in the case where transparency is not needed, but I imagine > this could have been me. > > Here is the change: > > > http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/renderer_host/render_widget_host_unittest.cc?revision=14378&view=markup > > - a > > On Thu, Apr 30, 2009 at 10:44 AM, Adam Langley wrote: > > > > On Thu, Apr 30, 2009 at 7:51 AM, Marc-Andre Decoste > wrote: > >>An alternative could be to send a bitmap the size of the union rect, > but > >> only paint the individual rects in it, and extract them individually on > the > >> other side of the IPC... But I wonder if it would be worth the added > >> complexity and risk... Unless I missed something (which is most probably > the > >> case :-)... > > > > Forgive me if I'm misunderstanding here. But is the problem that the area > of the > > union rectangle is significantly greater than the areas of the actually > damaged > > regions, thus we're painting too much? > > > > If that's the case, we could well change the PaintRect and ScrollRect > messages > > to carry a vector of rects and have them arranged in sequence in the > > TransportDIB. Since I'm currently to blame for much of the IPC painting > code, I > > can do this if it'll be of benefit. > > > > > > AGL > > > > > > > > > --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Need help in finding a performance problem...
Was this a recent regression? I made some changes in that area recently to support transparent webviews. I tried not to change anything in the case where transparency is not needed, but I imagine this could have been me. Here is the change: http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/renderer_host/render_widget_host_unittest.cc?revision=14378&view=markup - a On Thu, Apr 30, 2009 at 10:44 AM, Adam Langley wrote: > > On Thu, Apr 30, 2009 at 7:51 AM, Marc-Andre Decoste wrote: >> An alternative could be to send a bitmap the size of the union rect, but >> only paint the individual rects in it, and extract them individually on the >> other side of the IPC... But I wonder if it would be worth the added >> complexity and risk... Unless I missed something (which is most probably the >> case :-)... > > Forgive me if I'm misunderstanding here. But is the problem that the area of > the > union rectangle is significantly greater than the areas of the actually > damaged > regions, thus we're painting too much? > > If that's the case, we could well change the PaintRect and ScrollRect messages > to carry a vector of rects and have them arranged in sequence in the > TransportDIB. Since I'm currently to blame for much of the IPC painting code, > I > can do this if it'll be of benefit. > > > AGL > > > > --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Need help in finding a performance problem...
On Thu, Apr 30, 2009 at 7:51 AM, Marc-Andre Decoste wrote: > An alternative could be to send a bitmap the size of the union rect, but > only paint the individual rects in it, and extract them individually on the > other side of the IPC... But I wonder if it would be worth the added > complexity and risk... Unless I missed something (which is most probably the > case :-)... Forgive me if I'm misunderstanding here. But is the problem that the area of the union rectangle is significantly greater than the areas of the actually damaged regions, thus we're painting too much? If that's the case, we could well change the PaintRect and ScrollRect messages to carry a vector of rects and have them arranged in sequence in the TransportDIB. Since I'm currently to blame for much of the IPC painting code, I can do this if it'll be of benefit. AGL --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Need help in finding a performance problem...
Thanks Mike, Others can correct me if I'm wrong, but we do indeed call Layout on the webwidget (which will do the recursive layout of all frames, from the main_frame() down through its children) before actually painting from within RenderWidget::DoDeferredPaint(). And about the list of individual paint rects as opposed to the union rect, I think our multi-process architecture wouldn't like that too much... Sending a series of individual IPCs for each bitmap rect would most probably be overkill... An alternative could be to send a bitmap the size of the union rect, but only paint the individual rects in it, and extract them individually on the other side of the IPC... But I wonder if it would be worth the added complexity and risk... Unless I missed something (which is most probably the case :-)... Any other opinions on this? Thanks again! BYE MAD On Wed, Apr 29, 2009 at 4:30 PM, Mike Pinkerton wrote: > On Wed, Apr 29, 2009 at 3:39 PM, Marc-Andre Decoste > wrote: > > So I thought some people on this list might have some more hints to help > me > > pinpoint the culprit quicker... > > Until then, I keep digging... > > I'm pretty sure you've seen these, but from my discussions with Hyatt > on the Mac side, I'm pasting the things he told me to watch for: > > "Your repaint code really should not just be doing a union. That's > going to hurt your performance on DHTML and stuff. > Both Windows (HWNDs) and Mac (NSViews) will build up a region when you > do invalidates. When you draw you can then grab individual decomposed > rects from the overall region or use the unioned bounding box. > Look at WebHTMLView and how it has a drawRect and drawSingleRect methods." > > Though from talking to Darin a bit (our Darin), he was against > decomposing the rects since there was no obvious win given the async > nature of our repainting. I still think there's something here, but I > don't know enough about our layout and how we differ (purposely) from > WebKit to argue one way or the other. > > "Make sure that your paint method is actually doing the recursive > layout of all frames before actually painting so that (a) your layout > info is up to date and (b) any additional rects added by the layouts > can become part of that current paint. You'll probably want to study > the Windows WebView for this." > > Again, I'm ignorant of what we do here, though I'm pretty sure we > already do this. > > Hope that helps. > > -- > Mike Pinkerton > Mac Weenie > pinker...@google.com > --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Need help in finding a performance problem...
On Wed, Apr 29, 2009 at 3:39 PM, Marc-Andre Decoste wrote: > So I thought some people on this list might have some more hints to help me > pinpoint the culprit quicker... > Until then, I keep digging... I'm pretty sure you've seen these, but from my discussions with Hyatt on the Mac side, I'm pasting the things he told me to watch for: "Your repaint code really should not just be doing a union. That's going to hurt your performance on DHTML and stuff. Both Windows (HWNDs) and Mac (NSViews) will build up a region when you do invalidates. When you draw you can then grab individual decomposed rects from the overall region or use the unioned bounding box. Look at WebHTMLView and how it has a drawRect and drawSingleRect methods." Though from talking to Darin a bit (our Darin), he was against decomposing the rects since there was no obvious win given the async nature of our repainting. I still think there's something here, but I don't know enough about our layout and how we differ (purposely) from WebKit to argue one way or the other. "Make sure that your paint method is actually doing the recursive layout of all frames before actually painting so that (a) your layout info is up to date and (b) any additional rects added by the layouts can become part of that current paint. You'll probably want to study the Windows WebView for this." Again, I'm ignorant of what we do here, though I'm pretty sure we already do this. Hope that helps. -- Mike Pinkerton Mac Weenie pinker...@google.com --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
