- Dillo-dev - lists.mailman3.com

[experimental patch] excessive redraws (again)

by jcid＠dillo.org

Hi, > > [...] > > Back in 2005 we needed to test the prototype quickly, so it was > > reasonable to simply add, for instance, a queuedrawTotal() call > > when only a queuedraw() was needed and to postpone clipping until > > the basics were working. > > > > I believe this is what is asking for a review now. > > Yes sure. There might be places where we can replace the > queueDrawTotal(). Let's go through the code with this in mind. Funny how commenting out Widget::queueDrawArea() gives the same rendering (note that Widget::queueDraw() calls Widget::queueDrawArea()). When there are images a number in the hundred magnitude may be wasted. :-) -- Cheers Jorge.-

Feb. 8, 2008

[experimental patch] excessive redraws (again)

by jcid＠dillo.org

On Thu, Feb 07, 2008 at 09:50:00PM +0100, Johannes Hofmann wrote: > Hi, > > On Thu, Feb 07, 2008 at 05:23:40PM -0300, Jorge Arellano Cid wrote: > > - snip - > > > > > > > If I understand this correctly, the patch now is equivalent to > > > removing the calls to queueDrawTotal(), where asap == false, right? > > > > Right. > > > > But the idea is to queue when asap == false, and to set a delayed > > draw that is user adjustable in seconds (e.g. 0, 0.5, 1, 3, ...). > > Ok. I think we mostly agree on what needs to be done. My only > concern with your aproach is that I wonder why the new asap > parameter is needed: > When to call with asap == true and when to call it with asap == false? As a synonym for flush. For instance when and image finishes loading we can call with asap=true and if we want it to display incrementally, we can asap=true every update. asap=false can mean: "this operation is needed but can be delayed". > > > * Frequent redraws are good if they update the visible content. > > > E.g. when loading a page over a slow line. > > > Immediate feedback to the user is a great feature of dillo. > > > The screen flickering can be avoided with some double buffer > > > mechanism. > > > > Choosing refresh_delay=0 would give immediate feedback. > > > > refresh_delay=3 could yield low CPU usage and very good > > performance. > > Yes, that's exactly what my patch does. It delays the actual > drawing operation if there is other things to do (e.g. new data is > coming in). Yes, both patches attack the same problem after all. Once the code is cleaned, we can choose the one that fits better. I liked the flush/asap approach because it was putting unnecesary calls under the spotlight. It was also faster in some tests. Although this may change after the cleanup. > > > [...] > > > * It would be nice, if we could decide whether a resizing widget > > > affects the layout that is currently visible, so that we could > > > avoid redrawing in this case. This would avoid redrawing when > > > loading very long pages as the DragonFly package list. > > > But I don't think this is easy. > > > > Back in 2005 we needed to test the prototype quickly, so it was > > reasonable to simply add, for instance, a queuedrawTotal() call > > when only a queuedraw() was needed and to postpone clipping until > > the basics were working. > > > > I believe this is what is asking for a review now. > > Yes sure. There might be places where we can replace the > queueDrawTotal(). Let's go through the code with this in mind. OK. I also have yet to understand how it all fits together... (allocate/resize/draw) > > > Also we should agree on what we actually want to optimize. > > > I don't think CPU usage or number of redraws are enough. > > > Instead dillo should be usable and "feel fast" on fast and > > > slow or loaded systems (if CPU is available, why not use it?). > > > > I believe we can have it all! (as explained above). > > Same here :-) Good. -- Cheers Jorge.-

Feb. 7, 2008

[experimental patch] excessive redraws (again)

by Johannes.Hofmann＠gmx.de

Hi, On Thu, Feb 07, 2008 at 05:23:40PM -0300, Jorge Arellano Cid wrote: - snip - > > > > If I understand this correctly, the patch now is equivalent to > > removing the calls to queueDrawTotal(), where asap == false, right? > > Right. > > But the idea is to queue when asap == false, and to set a delayed > draw that is user adjustable in seconds (e.g. 0, 0.5, 1, 3, ...). Ok. I think we mostly agree on what needs to be done. My only concern with your aproach is that I wonder why the new asap parameter is needed: When to call with asap == true and when to call it with asap == false? > > > With this patch I see missing screen updates e.g. when resizing the > > window. > > Yes some glitches remain. There's still a need to add a few > queueDrawTotal(true) calls in necessary places so this is solved. > Once this is done, the amount of CPU usage decreases dramatically > and the user can choose a refresh rate. > > > First let me explain how I think the drawing and relayout is > > designed in dillo2. Please correct me if I'm wrong. > > Oh, I didn't design it so we're both exploring this area. > > > > * Frequent redraws are good if they update the visible content. > > E.g. when loading a page over a slow line. > > Immediate feedback to the user is a great feature of dillo. > > The screen flickering can be avoided with some double buffer > > mechanism. > > Choosing refresh_delay=0 would give immediate feedback. > > refresh_delay=3 could yield low CPU usage and very good > performance. Yes, that's exactly what my patch does. It delays the actual drawing operation if there is other things to do (e.g. new data is coming in). > > e.g. as place commented: > > > > You can try with the new patch. > > > > dillo-fltk immediate /* clear_double_buffer(); */ > > dillo-fltk 0:06 clear_double_buffer(); > > > > * queueResize() calls are put into the code whenever the layout > > might have changed without worrying too much about performance. > > To make this efficient, queueResize() or queueDrawTotal() just > > remembers drawing or resizing should be done at some time in > > the future. > > It looks like, but there are also some unimplemented functions > like startDrawing (), finishDrawing(), and flush(). Maybe they > were discarded, myabe left for future implementation, or left for > a second design-pass. > > > > * The idle function mechanism then does the actual work of resizing > > or redrawing. This way many calls to queueResize() can cause many > > resize operations, or they can be combined to just single resize > > depending on when the idle functions are called. > > When loading the same page, dillo can update the screen frequently > > if enough CPU is available, or less often, if CPU time is not > > available. > > > > I think this is an excellent concept :-) > > :) > > > > So what do we need to do? > > > > * Make the concept work properly. Here I think we need to improve > > the code where the idle functions are called so that multiple > > redraws are really combined especially if there is no CPU available. > > > > * Of course we still should reconsider each call to queueDrawTotal() > > or queueResize() and verify whether it is really necessary. > > However I think we should favor correctness over speed. That > > means, whenever the layout might have changed, we must call > > queueResize(). > > Sure. The patch is not complete. After adding the missing > flushes it should work OK. And refresh_delay would help to set > the desired behaviour. > > > * It would be nice, if we could decide whether a resizing widget > > affects the layout that is currently visible, so that we could > > avoid redrawing in this case. This would avoid redrawing when > > loading very long pages as the DragonFly package list. > > But I don't think this is easy. > > Back in 2005 we needed to test the prototype quickly, so it was > reasonable to simply add, for instance, a queuedrawTotal() call > when only a queuedraw() was needed and to postpone clipping until > the basics were working. > > I believe this is what is asking for a review now. Yes sure. There might be places where we can replace the queueDrawTotal(). Let's go through the code with this in mind. > > > Also we should agree on what we actually want to optimize. > > I don't think CPU usage or number of redraws are enough. > > Instead dillo should be usable and "feel fast" on fast and > > slow or loaded systems (if CPU is available, why not use it?). > > I believe we can have it all! (as explained above). Same here :-) Cheers, Johannes

Feb. 7, 2008

[experimental patch] excessive redraws (again)

by jcid＠dillo.org

Hi, On Wed, Feb 06, 2008 at 01:13:30PM +0100, Johannes Hofmann wrote: > Hi Jorge, > > On Mon, Feb 04, 2008 at 07:31:29PM -0300, Jorge Arellano Cid wrote: > > > > Well an experimental prototype was committed. > > The original idea here was to use this new API: > > > > void Textblock::flush (bool asap) > > void FltkViewBase::queueDrawTotal (bool asap) > > > > (asap = "As soon as possible") > > > > and to call the function inmediately when "asap" is true and to > > otherwise delay to a tunable number of seconds. > > > > After some tests it showed extraordinarily effective. For instance > > testing with the "All" directory from: > > > > http://chlamydia.fs.ei.tum.de/pub/DragonFly/packages/stable/DragonFly-1.10.… > > > > it was roughly using 2 to 3 times less CPU (in my notebook) than > > using the FltkPlatform::generalIdle() throttling patch. > > > > Some glitches remained, and while I was working on it, the test > > server above went down... :( > > > > I got a big surprise when after some more tests, it worked quite well > > when ignoring everything but calls with asap=true ! :-) > > > > For instance, now the code reads: > > > > void FltkViewBase::queueDrawTotal (bool asap) > > { > > static time_t lastRedraw = 0; > > > > //if (asap || time(0) > lastRedraw + 2) { > > if (asap) { > > redraw (DAMAGE_EXPOSE); > > lastRedraw = time(0); > > } > > } > > If I understand this correctly, the patch now is equivalent to > removing the calls to queueDrawTotal(), where asap == false, right? Right. But the idea is to queue when asap == false, and to set a delayed draw that is user adjustable in seconds (e.g. 0, 0.5, 1, 3, ...). > With this patch I see missing screen updates e.g. when resizing the > window. Yes some glitches remain. There's still a need to add a few queueDrawTotal(true) calls in necessary places so this is solved. Once this is done, the amount of CPU usage decreases dramatically and the user can choose a refresh rate. > First let me explain how I think the drawing and relayout is > designed in dillo2. Please correct me if I'm wrong. Oh, I didn't design it so we're both exploring this area. > * Frequent redraws are good if they update the visible content. > E.g. when loading a page over a slow line. > Immediate feedback to the user is a great feature of dillo. > The screen flickering can be avoided with some double buffer > mechanism. Choosing refresh_delay=0 would give immediate feedback. refresh_delay=3 could yield low CPU usage and very good performance. e.g. as place commented: > > You can try with the new patch. > > dillo-fltk immediate /* clear_double_buffer(); */ > dillo-fltk 0:06 clear_double_buffer(); > * queueResize() calls are put into the code whenever the layout > might have changed without worrying too much about performance. > To make this efficient, queueResize() or queueDrawTotal() just > remembers drawing or resizing should be done at some time in > the future. It looks like, but there are also some unimplemented functions like startDrawing (), finishDrawing(), and flush(). Maybe they were discarded, myabe left for future implementation, or left for a second design-pass. > * The idle function mechanism then does the actual work of resizing > or redrawing. This way many calls to queueResize() can cause many > resize operations, or they can be combined to just single resize > depending on when the idle functions are called. > When loading the same page, dillo can update the screen frequently > if enough CPU is available, or less often, if CPU time is not > available. > > I think this is an excellent concept :-) :) > So what do we need to do? > > * Make the concept work properly. Here I think we need to improve > the code where the idle functions are called so that multiple > redraws are really combined especially if there is no CPU available. > > * Of course we still should reconsider each call to queueDrawTotal() > or queueResize() and verify whether it is really necessary. > However I think we should favor correctness over speed. That > means, whenever the layout might have changed, we must call > queueResize(). Sure. The patch is not complete. After adding the missing flushes it should work OK. And refresh_delay would help to set the desired behaviour. > * It would be nice, if we could decide whether a resizing widget > affects the layout that is currently visible, so that we could > avoid redrawing in this case. This would avoid redrawing when > loading very long pages as the DragonFly package list. > But I don't think this is easy. Back in 2005 we needed to test the prototype quickly, so it was reasonable to simply add, for instance, a queuedrawTotal() call when only a queuedraw() was needed and to postpone clipping until the basics were working. I believe this is what is asking for a review now. > Also we should agree on what we actually want to optimize. > I don't think CPU usage or number of redraws are enough. > Instead dillo should be usable and "feel fast" on fast and > slow or loaded systems (if CPU is available, why not use it?). I believe we can have it all! (as explained above). -- Cheers Jorge.-

Feb. 7, 2008

[patch] fix HomeKey/EndKey behaviour

by jcid＠dillo.org

On Wed, Feb 06, 2008 at 06:21:23PM +0100, Johannes Hofmann wrote: > Hi > > the patch below makes HomeKey and EndKey work as it did in dillo1. > I think I had introduced the current behaviour that also adjusts the > x-position but it is pretty annoying on wide pages. Committed. -- Cheers Jorge.-

Feb. 7, 2008

[patch] fix HomeKey/EndKey behaviour

by Johannes.Hofmann＠gmx.de

Hi the patch below makes HomeKey and EndKey work as it did in dillo1. I think I had introduced the current behaviour that also adjusts the x-position but it is pretty annoying on wide pages. Cheers, Johannes diff -r 1c41f9a73971 dw/fltkviewport.cc --- a/dw/fltkviewport.cc Sun Feb 03 21:13:11 2008 +0100 +++ b/dw/fltkviewport.cc Wed Feb 06 18:16:41 2008 +0100 @@ -292,11 +292,11 @@ int FltkViewport::handle (int event) return 1; case HomeKey: - scrollTo (0, 0); + scrollTo (scrollX, 0); return 1; case EndKey: - scrollTo (canvasWidth, canvasHeight); /* gets adjusted in scroll() */ + scrollTo (scrollX, canvasHeight); /* gets adjusted in scroll() */ return 1; } }

Feb. 6, 2008

[experimental patch] excessive redraws (again)

by Johannes.Hofmann＠gmx.de

Hi Jorge, On Mon, Feb 04, 2008 at 07:31:29PM -0300, Jorge Arellano Cid wrote: > > Well an experimental prototype was committed. > The original idea here was to use this new API: > > void Textblock::flush (bool asap) > void FltkViewBase::queueDrawTotal (bool asap) > > (asap = "As soon as possible") > > and to call the function inmediately when "asap" is true and to > otherwise delay to a tunable number of seconds. > > After some tests it showed extraordinarily effective. For instance > testing with the "All" directory from: > > http://chlamydia.fs.ei.tum.de/pub/DragonFly/packages/stable/DragonFly-1.10.… > > it was roughly using 2 to 3 times less CPU (in my notebook) than > using the FltkPlatform::generalIdle() throttling patch. > > Some glitches remained, and while I was working on it, the test > server above went down... :( > > I got a big surprise when after some more tests, it worked quite well > when ignoring everything but calls with asap=true ! :-) > > For instance, now the code reads: > > void FltkViewBase::queueDrawTotal (bool asap) > { > static time_t lastRedraw = 0; > > //if (asap || time(0) > lastRedraw + 2) { > if (asap) { > redraw (DAMAGE_EXPOSE); > lastRedraw = time(0); > } > } If I understand this correctly, the patch now is equivalent to removing the calls to queueDrawTotal(), where asap == false, right? With this patch I see missing screen updates e.g. when resizing the window. > > (lastRedraw is not used). > > > The patch is much simpler than it looks. Many lines are just > for adding the extra parameter. > > Images on long pages still queue a resize that produces a total > redraw. i.e when an image outside the viewport resizes, the > viewport is redrawn causing flickering. > > This is not completely trivial because the image resize may > change the size of the parent widget, and the parent widget may > affect the viewable area. > > If this can somehow be avoided, for instance testing whether > the parent widget changes its size, it'd be great. > > This part of the code clearly needs a review. I wanted to > commit early so you guys have the chance to play with it. First let me explain how I think the drawing and relayout is designed in dillo2. Please correct me if I'm wrong. * Frequent redraws are good if they update the visible content. E.g. when loading a page over a slow line. Immediate feedback to the user is a great feature of dillo. The screen flickering can be avoided with some double buffer mechanism. * queueResize() calls are put into the code whenever the layout might have changed without worrying too much about performance. To make this efficient, queueResize() or queueDrawTotal() just remembers drawing or resizing should be done at some time in the future. * The idle function mechanism then does the actual work of resizing or redrawing. This way many calls to queueResize() can cause many resize operations, or they can be combined to just single resize depending on when the idle functions are called. When loading the same page, dillo can update the screen frequently if enough CPU is available, or less often, if CPU time is not available. I think this is an excellent concept :-) So what do we need to do? * Make the concept work properly. Here I think we need to improve the code where the idle functions are called so that multiple redraws are really combined especially if there is no CPU available. * Of course we still should reconsider each call to queueDrawTotal() or queueResize() and verify whether it is really necessary. However I think we should favor correctness over speed. That means, whenever the layout might have changed, we must call queueResize(). * It would be nice, if we could decide whether a resizing widget affects the layout that is currently visible, so that we could avoid redrawing in this case. This would avoid redrawing when loading very long pages as the DragonFly package list. But I don't think this is easy. Also we should agree on what we actually want to optimize. I don't think CPU usage or number of redraws are enough. Instead dillo should be usable and "feel fast" on fast and slow or loaded systems (if CPU is available, why not use it?). Cheers, Johannes

Feb. 6, 2008

[PATCH]: Table::apportionRowSpan()

by onepoint＠starurchin.org

On Fri, Feb 01, 2008 at 06:11:58PM -0300, Jorge Arellano Cid wrote: > On Fri, Feb 01, 2008 at 07:49:43PM +0000, Jeremy Henty wrote: > The "obvious right thing" may vary. For instance, what the > algorithm currently does is to proportionally apportion the extra > space (just as Firefox). Good point! I was thinking of sharing the space equally rather than proportionally, but proportionally is better. I think I'll be able to do that in O(n). I've put the rewrite on the back burner for now, as its priority has dropped since the actual breakage that starting me thinking along these lines has been fixed. Regards, Jeremy Henty

Feb. 5, 2008

[experimental patch] excessive redraws (again)

by place＠gobigwest.com

Jorge wrote: > Well an experimental prototype was committed. Looking at remote files now, it often doesn't draw anything at all until Eof.

Feb. 5, 2008

[patch] fix size of select widgets

by jcid＠dillo.org

Hi Johannes, On Mon, Feb 04, 2008 at 04:19:45PM +0100, Johannes Hofmann wrote: > Hi Jorge, > > On Sat, Feb 02, 2008 at 07:23:17PM -0300, Jorge Arellano Cid wrote: > > On Sat, Feb 02, 2008 at 07:27:20PM +0100, Johannes Hofmann wrote: > > > Here is a small patch to compute the size of select widgets similar > > > to the way it is computed for text input widgets. > > > The width is hardcoded to 10 * size of 'M' in the current font. > > > > I committed a different version that measures the string length > > (as dillo1 and FF). You can test with: > > > > http://www.dillo.org/bugtrack/Dinsert.html > > > > (adding a long string to a local copy is useful). > > > > I doubt that's the right way to do it, but at least it works. > > Please consider it experimental and give it a look. > > I don't think it is valid to compute the size in addItem(). The > style may change later, especially in the future with CSS. > I would propose to do the size computation in sizeRequest(), even if > that looks inefficient (patch below). > That way it is consistent with e.g. FltkLabelButtonResource. Committed with the following comment: // Looks like O(n^2) but the loop is executed one time -> O(n). :-) > While at it, I replaced the hardcoded 20 for the small rectangle on > the right with what FLTK uses for the width of the rectangle. Good. -- Cheers Jorge.-

Feb. 5, 2008