[Webkit-unassigned] [Bug 11031] Another crazy counters bug

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Nov 20 12:40:14 PST 2009


https://bugs.webkit.org/show_bug.cgi?id=11031





--- Comment #29 from Darin Adler <darin at apple.com>  2009-11-20 12:40:13 PST ---
Sounds like you're heading in the right direction. I do have a few thoughts.

(In reply to comment #28)
> I hate pedantic fixes that require bug filings and going through a whole review
> process just to add comments to code and change variable names which add no
> benefit to the user, and are beneficial to a developer only in the context of
> looking at a real user issue, so I put the comments and related fixes in this
> patch together with this algorithm fix hopefully the reviewer (likely Darin)
> will not mind that.

It's extremely helpful to have bug fixes that have only the bug fix. Separate
refactoring patches help achieve that and make the process of fixing bugs much
safer and more deliberate.

I'm sorry you hate those patches.

> Since I had a lot of explaining to do, I used line
> breaks to separate main ideas, try to make the comment more readable while
> keeping it pretty condensed.

I found the formatting of the comment distracting. Starting the new sentences
on new lines didn't make it easier for me to understand it.

> as far as including the initial updateCounter in the loop, I
> was trying to save an unnecessary comparison, since this function is called for
> every DOM node insertion I was trying to make it as quick as possible if the
> node being inserted and its descendants have no counter directives.

If the extra branch at the start of loop is a concern, other possible idioms
are a do/while loop or an infinite loop with an explicit break.

> As I explained in my response to Shinchiro Hamaji, this work is usually not
> that much

In a hot function like this, to turn the guess of "usually not that much" into
something we can make decisions based on, we'd typically do measurement to see
what effect the code change has on performance. Trying with something we think
is likely to be the worst case of additional overhead.

> The code above appears to replace 4 lines, but in reality it adds totally new
> functionality while optimizing those 4 lines to run faster.

If you're saying this new structure should be judged as a performance
optimization, then it should come by itself in a separate patch, with
measurement of the speedup.

> I read an article in a respected Software Development journal where a guy
> published a breadth first graph travesal algorithm optimized for the Cell
> processor. The algorithm had 5000 lines versus the usual about 50. But was
> running an order of magnitude faster on the same hardware as the vanilla
> algorithm.

If you're going to cite this article as evidence that your approach is good,
then you'll need to measure actual speedup. The guy in that article did.

I did notice the way your patch cut down on work in various cases by reusing
the results of if statements. Consider that readability of the code is critical
although minimizing the number of lines is not. If we have to make things
significantly less readable, then we need a *measurable* speedup to
counterbalance that cost.

> I read somewhere in the style
> guidelines that long lines are bad and I was trying to comply with that.

We should fix that guideline; it's not really our WebKit style. Could you cite
the specific text? I'd like to correct it.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list