92630 - list number conversion (1 -> "i", 2 -> "ii" ...) done during paint

Status: RESOLVED WONTFIX

(Core :: Layout: Block and Inline, defect)

Description

[Daniel Bratell]

In nsBulletFrame, the list number conversion is done during paint. This is no 
noop operation for large roman numbers for instance, so the result should be 
cached or computed during reflow.

I saw this while working on the Integer->Roman conversion and testing with big 
numbers. The window become unresponsive and almost impossible to scroll. I've 
changed my implementation to not handle as big numbers (Try to write 2 billions 
in roman digits without using the boxed variants) to make it faster, but this 
calculations shouldn't be repeated all the time.

Comment 1

[karnaze (gone)]

Reassigning to attinasi.

Comment 2

[Daniel Bratell]

In the patch in bug 92065 I limited myself to numbers > 500000 and then the 
window is responsive at least on my (fast) computer.

Comment 3

[Marc Attinasi]

Accepting.

Comment 4

[Boris Zbarsky [:bzbarsky]]

.

Comment 5

[Boris Zbarsky [:bzbarsky]]

So the goal here would be to cache the string and only reset it during reflow...

See nsBulletFrame.cpp, the Paint() and Reflow() methods.

Comment 6

[Eli Friedman]

Attachment: Patch v1

FYI, I'm new at patching code.

Comment 7

[Eli Friedman]

Who should I ask for r=?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  PRBool mRegularStringType;

Make it a PRPackedBool.

It's good to make patches with the -t option to get the function names in the diff.

The code looks great but I am rather surprised that it's necessary. How sure are
we that Daniel was right about the performance issues ... and that he's still
right? I'm really surprised because the code to compute the strings seems a lot
simpler than the code that paints or measures them.

Comment 9

[Eli Friedman]

I should have tried that before I posted the patch; actually, I can't tell the
difference in rough perf testing.  It increases footprint for long lists, and
probably slows it down, although I couldn't tell.  Oh well, I should check next
time.

That code needs some other changes I was looking at; maybe I'll try to patch a
couple of those.

Comment 10

[Eli Friedman]

Overall, not worth doing, so marking WONTFIX.