The attached patch is only version 1 (don't commit it yet, its not thoroughly tested and I want to spend some more time and make more changes until I call it ready for production).

I did some poor-man's profiling with the chm viewer lhelp (hit pause during the slowness look at the callstack, catch it in the act, repeat this a few times [effectively the same thing that a sampling profiler like oprofile would do]). During scrolling of long pages it seemed to spend a lot (almost all) of its time in the same procedure, looking up css property names in a TStringList with IndexOf(). TStringList.IndexOf() does a linear search, so I thought why not replace this with a TFPObjectHashTable. All it has to do is store a list of objects by name and all it is needed for is looking them up by name thousands and thousands of times.

After doing this (see the attached patch, its only a few lines) the scrolling seems a hundred times faster! Really! Try it with rtl.chm and look up the help for Format(), its a very long page, it was basically unscrollable before. I'm totally excited! This is the first time I have seen lhelp being actually usable! This is the reason I could not wait to post this patch.

I'm now going to see if I can find a few more TStringlists to replace this way.

Bernd
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lhelp_turbopower_ipro_slowness_v1.patch
Type: text/x-diff
Size: 2082 bytes
Desc: not available
URL: <http://lists.lazarus.freepascal.org/pipermail/lazarus/attachments/20111224/1f3ecdb1/attachment.bin>

I believe the use of the unit "contnrs" belongs inside the $ifdef IP_LAZARUS (the entire css functionality seems to be lazarus-only anyways, so no ifdefs needed in ipcss.inc)

I didn't find any other TStringList yet with such heavy usage of IndexOf().

Bernd
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lhelp_turbopower_ipro_slowness_v2.patch
Type: text/x-diff
Size: 2055 bytes
Desc: not available
URL: <http://lists.lazarus.freepascal.org/pipermail/lazarus/attachments/20111224/4fb6ca87/attachment.bin>

I removed an unneeded LowerCase(), the css and html parsers will already deliver only lowercase element names anyways.

Bernd
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lhelp_turbopower_ipro_slowness_v3.patch
Type: text/x-diff
Size: 2757 bytes
Desc: not available
URL: <http://lists.lazarus.freepascal.org/pipermail/lazarus/attachments/20111225/f558c794/attachment-0001.bin>

Bernd wrote

I removed an unneeded LowerCase(), the css and html parsers will already deliver only lowercase element names anyways. Bernd

This patch makes a noticeable difference on my computer as well. I applied it in r34400.

Thanks,

Andrew

a noticeable difference as in how? faster processing or that more "nodes" are being properly processed and placed into the resulting output files?

2011/12/25 waldo kitty :

a noticeable difference as in how? faster processing or that more "nodes" are being properly processed and placed into the resulting output files?

Faster scrolling. I am using it on an old Pentium-M with 1.3GHz and before there were pages that were effectively "unscrollable" for me. One extreme example that makes it very obvious is if you look up the help for the RTL function Format() in rtl.chm. Before the patch it took around 1 second for every scroll event, If I used the mouse wheel then the GUI was frozen for 1s, then displaying it scrolled 3 lines up, then frozen again for one second, then another 3 lines, etc. Now I can scroll this same page very smoothly, no freezing and waiting anymore.

BTW in my first post I mentioned css property names, this was wrong, this was before I digged a bit deeper into it to understand what this function actually needed for: It is looking up html tag names in a list of names to get a reference to the css property object that is attached to a tag name. for example there might exist a css directive like p: {color: #00ff00;} then while parsing the css it will use this function to look up the property object for 'p' and set its FColor to #00ff00. Later when rendering the page it will again use the same function on every node in the html tree that it encounters to get the properties object for this node (for every <p> in the html (and for any other tag) it will again up the tag name to retrieve the property object (TCSSProps object) to know the node's color (and other properties)).

For some reason this is done on *every* scroll event again and again. If I understand it correctly then after the HTML is parsed then there exists a tree of TIpHtmlNode objects and each node holds a reference to the appropriate TCSSProps object, it should not be needed to look them up more than once for each node upon construction of this node unless for some reason the entire html tree is is destroyed and built up from scratch again and I don't see why this should be necessary on every scroll event. The next step should be to find out what exactly is going on during a scroll event that causes this to happen every time.

Bernd

2011/12/25 Bernd :

every scroll event. The next step should be to find out what exactly is going on during a scroll event that causes this to happen every time.

Its a bit complicated. It looks up not only the tag name but also "tag:hover", "tag.class", ".class" and id. And then it merges them all one after the other into the properties of this tag. And then after merging these properties this function does something very strange: It returns one of these 5 property objects back to the caller but it is not clear which one and for what purpose and also it does not load tagname.classname if it already has successfully loaded tagname and other such strange things, very confusing.

I have therefore rewritten the LoadCSSProps() method from scratch and also added a method to merge two TCSSProps into one (and then let LoadCSSProps() return an obect with all CSS styles combined) and also some caching of all these props objects to avoid all the lookups by name.

The patch is not easy to read without applying because the old and the new version of LoadCSSProps() has been interleaved with each other every 5 lines although they do not have much in common.

This patch will probably break some things (or maybe also fix some old bugs?) I cannot see any visual difference in the lhelp application. Maybe the person who wrote all this should now say something before this is blindly applied. This patch changes some of the control flow and constitutes only my interpretation of what I believe LoadCSSProps() was initially supposed to do. But the whole ipro package is a *lot* of code and so it is quite possible that I misunderstood parts of the code.

Bernd
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lhelp_turbopower_ipro_cssprops_caching_v1.patch Type: text/x-diff
Size: 14113 bytes
Desc: not available
URL: <http://lists.lazarus.freepascal.org/pipermail/lazarus/attachments/20111225/2c6cac9b/attachment-0001.bin>

ACK, a direct reference to the CSS object should be stored.

The next step should be to find out what exactly is going on during a scroll event that causes this to happen every time.

The font size can affect the height of the HTML items, which is required during scrolling.

DoDi

2011/12/25 Bernd :

I have therefore rewritten the LoadCSSProps() method

Please ignore this last patch from me, It is still wrong, LoadCSSProps() now doesn't return what some of its callers would expect from it (whatever that might be, because it didn't make much sense anyways). There must be a better solution. IMHO the original version is wrong as well but it needs more work to fix this properly.

2011/12/26 Hans-Peter Diettrich :

But shouldn't the work of loading and applying all the different CSS properties of that node only be needed once after parsing is complete and not each time it is rendered? Ideally I think this entire method (LoadCSSProps) should not be called during render. If I understand it correctly then all it is supposed to do is to find all the different CSS props that apply to a node of this type and class and merge them in the correct order into the props object of that node. After that is done (once) it could be rendered. I wouldn't expect any of the CSS properties (or any properties at all) to *change* between two consecutive renders of the same document.

Bernd

ACK

I'm not sure whether GUI effects (e.g. highlight item under the mouse...) can cause differences in the applicable CSS properties of an item. But even then the rendering and scrolling should not be slower than in an ordinary browser.

DoDi

2011/12/26 Hans-Peter Diettrich :

Apparently somebody already tried to make this function exit immediately when this method is called a second time (when one would assume all its work has been done already) but for some reason this did not work as expected and has strange side effects. The problem (and the code involved in creating parts of the problem) is *much* more comlex than I thought it should be.

And also I am 100% sure there is something fundamentally wrong with this method itself, especially the following:

* the logic behind the control flow inside this method (when to apply which CSS)
* the way it returns only one of the 5 CSS objects it processes
* the purpose of returning this object at all (and how and for what it is used by its caller)
* the argument list containing Owner and Prop which are already properties of this class
* the existence of obvious hacks/workarounds without any comments
* comments in nearby/related parts of the code indicating that unwanted things *do* happen for *unknown* reasons.

I think I need to study the code much longer and more carefully to understand what exactly is going on here (and why). Now after I have seen this code I cannot ever use it again without permanently having to think about it in the back of my mind how it seems to be wrong and how to fix it. Even if it actually works (or seems to work). Eventually I will need to find a solution for this.

Or to quote Deep Thought: "my circuits are now irrevocably committed to calculating the answer to the [...] Question"

I could be mistaken, but it is common in javascript to change a class of a node (or add/remove different classes) on the fly. That would require a new render and a new gathering of css props for that node(tree).

kind regards,
Dimitri Smits

2011/12/26 Bernd :

Eventually I will need to find a solution for this.

I have made a new patch and put it into the bug tracker. This one should give another (small) speed improvement and solve some of the above mentioned inconsistencies and LoadCSSProperties() is now correct and easier to understand:
http://bugs.freepascal.org/view.php?id=20975

Bernd