May 2008 - Dillo-dev - lists.mailman3.com

[patch] dw2 memory reduction

by jcid＠dillo.org

On Sun, May 25, 2008 at 10:08:12AM +0200, Johannes Hofmann wrote: > Hi, > > On Sat, May 24, 2008 at 04:39:42PM -0400, Jorge Arellano Cid wrote: > > On Sat, May 24, 2008 at 02:58:08PM +0200, Johannes Hofmann wrote: > > > Hi, > > > > > > attached is patch to reduce the memory usage of dw2: > > > > Excellent! > > (AFAIU this is near 30% memory usage reduction in dw2) > > > > Committed. > > > > > > > > * Styles are now reused if they are equal. This brings down the > > > number of Style objects on the mysql page from around 180000 to > > > 23000 and reduces memory usage by about 25M at the cost of an > > > additional HashTable. > > > Maybe the hash table should be part of fltkplatform > > > as the hash tables for Color and Font? > > > There would be even more sharing of styles possible, if > > > x_link would not be part of style. On the other hand about 3M for > > > styles on the huge mysql page is pretty ok. > > > > > > * Size of struct Word is reduced by 4 integers by using shorts. I > > > know that this is questionable, but I think in the case of struct > > > Word that can be allocated a lot it is worth the hassle. > > > Please check whether my assumption that space lengths and word > > > lengths can be stored in shorts are valid in general. > > > > It looks OK at first sight, but to be sure it'd be good to know > > what happens when the limit is surpassed. > > Just tried it on a page with 40000 char long word. With short for > hlStart/hlEnd, selection is - obviously - no longer possible. > That's clearly a regression. > Please revert with attached patch. A max of 32K for a single word, looks quite reasonable to me. I meant, if the browser doesn't crash, and fails gracefully, why not. BTW, it can be extended to 64K with unsigned, by using 65535 as sentinel value (instead of -1), but again, is it worth? It'd be good to know how much "struct Word" is reduced in size (after compiler alignment), and how much it impacts overall memory usage. I mean if struct word is reduced 20% and that yields a 4% overall memory reduction, it may be not worth complicating the data type. OTOH, ... > > BTW, "unsigned short" looks like a good idea for origSpace and effSpace. Committed the "unsigned short" part. -- Cheers Jorge.-

May 25, 2008

Fuzzing dillo

by 4winter＠informatik.uni-hamburg.de

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hi :) I was playing around with Michal Zalewskis html fuzzer 'mangle' trying to crash dillo and succeeded :). Furthermore I was able to reduce the ~100k page that was triggering a NULL pointer dereferencing to three tags (see the attached page). Some digging around (html.cc is a monster ^^) revealed that style_attrs.setBorderColor ( Color::createShaded(HT2LT(html), style_attrs.backgroundColor->getColor())); in Html_tag_open_table was failing due to the fact that style_attrs.backgroundColor was NULL. Html_tag_open_select sets the backgroundColor to NULL and this is inherited by the table tag since it's a child of the select tag. Greping through the code shows that there are at least two other functions that set backgroundColor (and color) to NULL (Html_tag_open_input and Html_tag_open_isindex) and twelve locations calling getColor() on either color or backgroundColor. I have no idea how to _properly_ fix this issue. Furthermore I'd suggest to break down html.cc into several files. It is about 6000 lines long (-> hard to dig into) and it takes gcc roughly five seconds to compile on my box. Justus - -- gpg key fingerprint: C82D 382A AB38 1A54 5290 19D6 A0F9 B035 686C 6996 -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFIOUmmoPmwNWhsaZYRAprIAJ9X9HSzc4Isx+NyFWxlfSFPrwj/VACdF8GD x67scPvD6FCrOezl69NbaHA= =z0iv -----END PGP SIGNATURE-----

May 25, 2008

[patch] dw2 memory reduction

by Johannes.Hofmann＠gmx.de

Hi, On Sat, May 24, 2008 at 04:39:42PM -0400, Jorge Arellano Cid wrote: > On Sat, May 24, 2008 at 02:58:08PM +0200, Johannes Hofmann wrote: > > Hi, > > > > attached is patch to reduce the memory usage of dw2: > > Excellent! > (AFAIU this is near 30% memory usage reduction in dw2) > > Committed. > > > > > * Styles are now reused if they are equal. This brings down the > > number of Style objects on the mysql page from around 180000 to > > 23000 and reduces memory usage by about 25M at the cost of an > > additional HashTable. > > Maybe the hash table should be part of fltkplatform > > as the hash tables for Color and Font? > > There would be even more sharing of styles possible, if > > x_link would not be part of style. On the other hand about 3M for > > styles on the huge mysql page is pretty ok. > > > > * Size of struct Word is reduced by 4 integers by using shorts. I > > know that this is questionable, but I think in the case of struct > > Word that can be allocated a lot it is worth the hassle. > > Please check whether my assumption that space lengths and word > > lengths can be stored in shorts are valid in general. > > It looks OK at first sight, but to be sure it'd be good to know > what happens when the limit is surpassed. Just tried it on a page with 40000 char long word. With short for hlStart/hlEnd, selection is - obviously - no longer possible. That's clearly a regression. Please revert with attached patch. > > BTW, "unsigned short" looks like a good idea for origSpace and effSpace. Adjusted in patch. Cheers, Johannes

May 25, 2008

[BUG]: Redirect to previous page + Reload crashes

by corvid＠lavabit.com

Jeremy wrote: > On Sat, May 24, 2008 at 08:04:19PM +0100, Jeremy Henty wrote: > > > Reviewing the rest of nav.c I think there is a similar potential > > problem with Nav_stack_insert() , which also modifies bw->nav_stack > > . > > On second thoughts, Nav_stack_insert() seems broken. It frees the > pointer at the insertion point, so it clearly expects the new item to > replace the old. But then it calls dList_insert_pos() which does > *not* replace the item at the insertion point, instead it extends the > list and moves everything up to make space. So now we have a freed > pointer in bw->nav_stack . > > Either Nav_stack_insert() should not be freeing the pointer or it > should call a dList_*() function that replaces the item it has freed. > Which? Since it looks like Nav_stack_truncate() already got rid of everything at stack_idx and beyond, maybe Nav_stack_insert() could be replaced by a Nav_stack_append() function.

May 25, 2008

[BUG]: Redirect to previous page + Reload crashes

by onepoint＠starurchin.org

On Sat, May 24, 2008 at 08:04:19PM +0100, Jeremy Henty wrote: > Reviewing the rest of nav.c I think there is a similar potential > problem with Nav_stack_insert() , which also modifies bw->nav_stack > . On second thoughts, Nav_stack_insert() seems broken. It frees the pointer at the insertion point, so it clearly expects the new item to replace the old. But then it calls dList_insert_pos() which does *not* replace the item at the insertion point, instead it extends the list and moves everything up to make space. So now we have a freed pointer in bw->nav_stack . Either Nav_stack_insert() should not be freeing the pointer or it should call a dList_*() function that replaces the item it has freed. Which? Regards, Jeremy Henty

May 25, 2008

[patch] dw2 memory reduction

by Johannes.Hofmann＠gmx.de

On Sat, May 24, 2008 at 04:39:42PM -0400, Jorge Arellano Cid wrote: > On Sat, May 24, 2008 at 02:58:08PM +0200, Johannes Hofmann wrote: > > Hi, > > > > attached is patch to reduce the memory usage of dw2: > > Excellent! > (AFAIU this is near 30% memory usage reduction in dw2) > > Committed. > > > > > * Styles are now reused if they are equal. This brings down the > > number of Style objects on the mysql page from around 180000 to > > 23000 and reduces memory usage by about 25M at the cost of an > > additional HashTable. > > Maybe the hash table should be part of fltkplatform > > as the hash tables for Color and Font? > > There would be even more sharing of styles possible, if > > x_link would not be part of style. On the other hand about 3M for > > styles on the huge mysql page is pretty ok. > > > > * Size of struct Word is reduced by 4 integers by using shorts. I > > know that this is questionable, but I think in the case of struct > > Word that can be allocated a lot it is worth the hassle. > > Please check whether my assumption that space lengths and word > > lengths can be stored in shorts are valid in general. > > It looks OK at first sight, but to be sure it'd be good to know > what happens when the limit is surpassed. > > BTW, "unsigned short" looks like a good idea for origSpace and effSpace. > > > Also memory usage gets pretty high (250M with images disabled). > > I'm experimenting with reducing the size of struct Word in > > textblock.hh by moving the highlight stuff out of struct Word and > > into Textblock. This brings memory usage down to 199M. > > However, I don't know the highlighting stuff enough to be sure I > > don't break anything. This probabely falls into the > > "premature optimization" category anyway. > > hlStart and hlEnd use -1 as a value, so they can't be unsigned. > I don't know whether they can be taken out, and how much impact > it can have (alignment issues are strange sometimes). I have a version here where hlStart and hlEnd are taken out of struct Word. Instead I have just one hlStart and hlEnd containing the also the word index in Textblock. This is under the assumption that highlighting is only possible for one contiguous region per layer. Unfortunately this patch is still in the "works almost" state. Cheers, Johannes

May 24, 2008

[patch] dw2 memory reduction

by jcid＠dillo.org

On Sat, May 24, 2008 at 02:58:08PM +0200, Johannes Hofmann wrote: > Hi, > > attached is patch to reduce the memory usage of dw2: Excellent! (AFAIU this is near 30% memory usage reduction in dw2) Committed. > > * Styles are now reused if they are equal. This brings down the > number of Style objects on the mysql page from around 180000 to > 23000 and reduces memory usage by about 25M at the cost of an > additional HashTable. > Maybe the hash table should be part of fltkplatform > as the hash tables for Color and Font? > There would be even more sharing of styles possible, if > x_link would not be part of style. On the other hand about 3M for > styles on the huge mysql page is pretty ok. > > * Size of struct Word is reduced by 4 integers by using shorts. I > know that this is questionable, but I think in the case of struct > Word that can be allocated a lot it is worth the hassle. > Please check whether my assumption that space lengths and word > lengths can be stored in shorts are valid in general. It looks OK at first sight, but to be sure it'd be good to know what happens when the limit is surpassed. BTW, "unsigned short" looks like a good idea for origSpace and effSpace. > Also memory usage gets pretty high (250M with images disabled). > I'm experimenting with reducing the size of struct Word in > textblock.hh by moving the highlight stuff out of struct Word and > into Textblock. This brings memory usage down to 199M. > However, I don't know the highlighting stuff enough to be sure I > don't break anything. This probabely falls into the > "premature optimization" category anyway. hlStart and hlEnd use -1 as a value, so they can't be unsigned. I don't know whether they can be taken out, and how much impact it can have (alignment issues are strange sometimes). > * SimpleVector no longer preallocates memory if it's not necessary. > This actually saves some megabytes here! > > With these changes memory usage on the mysql page is 179M here. Great. -- Cheers Jorge.-

May 24, 2008

remaining preferences

by corvid＠lavabit.com

# Dicache is where the Decompressed Images are cached (not the original ones). # If you have a lot of memory and a slow CPU, use YES, otherwise use NO use_dicache The code always behaves like use_dicache=NO /* * Unrefs the counter of a dicache entry, and _if_ no DwImage is acessing * this buffer, then we call Dicache_free to do the dirty job. */ void a_Dicache_unref(const DilloUrl *Url, int version) { DICacheEntry *entry; if ((entry = Dicache_get_entry_version(Url, version))) { /*if (--entry->RefCount == 0 && (entry->next || !prefs.use_dicache)) {*/ if (--entry->RefCount == 0) { Dicache_remove(Url, version); } } } Perhaps this was done for a reason. # If you prefer oblique over italic fonts, uncoment next line use_oblique FLTK doesn't know about oblique. I don't know whether anybody cares enough to change that. I don't. # Show tooltip popup for images? # Note: We use the "title" attribute and not "alt". # More info at: http://bugzilla.mozilla.org/show_bug.cgi?id=25537 show_tooltip I did the easy part of tooltips long ago, but I haven't been able to care enough to want to do the fltk part. # Set this to YES, if you want to limit the word wrap width to the vieport # width (may be useful for iPAQ) limit_text_width It doesn't do anything. I don't think it's ever done anything. It sure would be nice if it did something. But I don't have the expertise to change that. # Here you can choose to hide some widgets of the dillo panel... show_menubar Not currently used. Meaningless with the current panel-cycling with right click on the search button (which I imagine is not going to be the permanent way to do that). # Enabling this will restrain OpenUrl and FindText, but may be required # for the ION window manager. transient_dialogs from fltk::Window::child_of(): Tell the system that this window will not have an icon, it will dissappear and reappear when \a parent is iconized or shown, and it is forced to always be above \a parent. On X this is called a "Transient window", and Windows calls this a "overlapping child". \a parent is different than the parent(), which must be zero. (and down in fltk::CreatedWindow::create, there's a if (window->child_of() && window->child_of()->shown()) XSetTransientForHint(xdisplay, x->xid, window->child_of()->i->xid);) If Justus gets rid of the dialogs, obviously the pref can be removed. Enabling this means making our own dialogs for message(), choice(), input(). # When filling forms, our default behaviour is to submit on enterpress, # but only when there's a single text entry (to avoid incomplete submits). # OTOH, if you have to fill the same form lots of times, you may find # useful to keep away from the mouse by forcing enter to submit. enterpress_forces_submit The code currently always submits on enter. I like it that way and didn't realize that it wasn't supposed to.

May 24, 2008

Q: why does Nav_stack_clean() free the data and Nav_stack_truncate() not?

by onepoint＠starurchin.org

While reviewing nav.c to figure out the reload crash bug I noticed something that seems inconsistent: both Nav_stack_truncate() and Nav_stack_clean() remove items from the navigation stack, yet Nav_stack_truncate() does not free the data it removes and Nav_stack_clean() does. Is this a bug? If not, what haven't I understood? Regards, Jeremy Henty

May 24, 2008

[BUG]: Redirect to previous page + Reload crashes

by onepoint＠starurchin.org

On Sat, May 24, 2008 at 03:22:10AM +0000, corvid wrote: > Jeremy wrote: > > Dillo crashes as follows: > > * Point Dillo at http://starurchin.org:8080/dillo/here > > * Click the "There" link. Dillo is redirected back to the "Here" > > page. > > * Click the "Reload" button. > > * Dillo crashes inside a_Nav_reload() (nav.c) at the line "if > > (URL_FLAGS(url) & URL_Post) {" because url is NULL. > > The problem went away when I tried the attached change, > but I may be treating the symptom rather than the disease. > diff -pur dillo2/src/nav.c dillo2-cur/src/nav.c > --- dillo2/src/nav.c 2008-05-10 20:02:22.000000000 +0000 > +++ dillo2-cur/src/nav.c 2008-05-24 03:03:50.000000000 +0000 > @@ -185,6 +185,8 @@ static void Nav_stack_clean(BrowserWindo > void *data = dList_nth_data (bw->nav_stack, i - 1); > dList_remove_fast (bw->nav_stack, data); > dFree(data); > + if (bw->nav_stack_ptr >= a_Nav_stack_size(bw)) > + bw->nav_stack_ptr = a_Nav_stack_size(bw) - 1; > } > } > I think this addresses the real problem, which is that bw->nav_stack_ptr is an index into bw->nav_stack , so if we modify the latter we must ensure the former is valid. Reviewing the rest of nav.c I think there is a similar potential problem with Nav_stack_insert() , which also modifies bw->nav_stack . Shouldn't it increment bw->nav_stack_ptr if it is after the insertion point? Otherwise the displayed item might change unexpectedly. Regards, Jeremy Henty

May 24, 2008