344778 - Optimize ASCII line breaking

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

By bug 255990. The tp value of tinderbox was damaged. We should optimize the line breaking algorithm.

Simple testcase is here:
http://www.d-toybox.com/mozilla/testpages/testcasesforbenchmark.zip

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'll post new result of the page loading score with the test tool of mozilla.org.

Comment 3

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

this looks good...

would be interesting to see how many times we end up calling CanBreakBetweenLatin1 on English pages vs the number of times we avoid calling CanBreakBetweenlatin1 (i.e., how often this optimization succeeds).

I'd also like to see the microbenchmark results for English Web page text with this optimization.

Another thing that'd be interesting is to run the microbenchmark on English text of spaces and letters only (so CanBreakBetweenLatin1 is never called), and compare two versions of the code: this patch, and this patch but with chIsSymbol set to just PR_TRUE. That would tell us whether it's worth optimizing those conditionals (which may not be predicted very well by the CPU).

Comment 4

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

BTW it would be nice if we could automatically test that our optimization isn't changing anything. Perhaps you could add an NS_ASSERTION to check that if the optimization is used, then CanBreakBetweenLatin1 would have returned PR_FALSE.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> Another thing that'd be interesting is to run the microbenchmark on English
> text of spaces and letters only (so CanBreakBetweenLatin1 is never called), and
> compare two versions of the code: this patch, and this patch but with
> chIsSymbol set to just PR_TRUE. That would tell us whether it's worth
> optimizing those conditionals (which may not be predicted very well by the
> CPU).

     patched build  patched but set to TRUE
AVG.       19028.0    19243.9
MED.       18953.0    19188.0

the test case is the English test case of comment 0. the score is 'ms'.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The current patch has many lossy cases. I'll add more checking.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.0

This is using static table for breaking check. This is faster than previous patch.

Bechmark score (simple English)(ms):
       normal  patched
AVG.  19356.3  18890.5
MED.  19461.0  18828.0

Comment 8

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

The result before bug 255990 was fixed was about 10,000, right?

What exactly does this benchmark do? Can you put the code somewhere I can see it?

Comment 9

[Jungshik Shin]

(In reply to comment #8)
> The result before bug 255990 was fixed was about 10,000, right?

No, it's around 18700. See bug 255990 comment #76. Masayuki reported there that he got 18804 without this optimization. Here, it's now 18953 with this optimization(all in median). Apparently, his numbers given in bug 255990 can't be compared directly with numbers given here. All three cases (with no patch at all, with bug 255990 patch alone and with optimization) need to be compared under identical conditions. Moreover, just giving median/average doesn't tell much. Masayuki, why don't you do t-test (every decent spreadsheet offers an easy way to do that) and give p-number?  

BTW, using our (mozilla.org) test tools (bug 255990 comment #59) is better than relying on his synthetic test cases, I believe. Note that our test web pages are virtually all in English. 

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #9)
> Masayuki, why don't you do t-test (every decent spreadsheet offers an
> easy way to do that) and give p-number?  

I cannot understand here. What means 't-test' and 'p-number'?

> BTW, using our (mozilla.org) test tools (bug 255990 comment #59) is better than
> relying on his synthetic test cases, I believe. Note that our test web pages
> are virtually all in English. 

Yeah, of course. I'll test by it ASAP.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

backed-out the original patch. I'll retry to fix it with UAX#14. If you have some ideas and suggestions, please tell me.

-> FIXED(by backed-out)