[webkit-reviews] review denied: [Bug 4789] so slow it feels like a hang calling UCFindTextBreak() tons of times at forum.presence-pc.com : [Attachment 3721] Make breakability testing run in linear time

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied opendarwin.org at mitzpettel.com's
request for review:
Bug 4789: so slow it feels like a hang calling UCFindTextBreak() tons of times
at forum.presence-pc.com
http://bugzilla.opendarwin.org/show_bug.cgi?id=4789

Attachment 3721: Make breakability testing run in linear time
http://bugzilla.opendarwin.org/attachment.cgi?id=3721&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
This is great! We've needed a fix like this for so long!

In RenderBlock::findNextLineBreak, it seems unfortunate that
nextBreakablePosition gets called more often than it needs to. For example,
it's called when isPre is true, and when the character is not '\n'.  I think
that could easily be fixed with a helper function that does both the "pos >
nextBreakable" work and the "pos == nextBreakable" check all in one, that is
called inside the expression.

Once you write that function, you might be able to use it in
RenderText::calcMinMaxWidth too, altough that code doesn't seem to do extra
nextBreakablePosition calls.

The expression "i+wordlen" appears so many times in the while loop in
RenderText::calcMinMaxWidth that I think it's worth changing the loop to use
something more like "j" and compute wordlen afer the loop. It's also not good
that the current character is fetched four different times. Lets rewrite that
loop as long as we're changing the code. I'm not certain it's a hot code path,
but it seems it could also be easier to read if redone a bit.

Frankly, re-getting str->s and str->l over and over again is aso pretty weak.
It's a memory access every time that might be avoided if we just put those
values into local variables.

Setting this to review- because of the RenderBlock::findNextLineBreak issue.
Please consider the other comments as suggestions. Maybe those changes would be
best left to another patch -- I don't feel strongly either way.