201670 - nsCollation::UnicodeToChar should cache aCharset, not mEncoderCharsetAtom

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[[e-mail]]

Wondering why it takes Mail 3-4 seconds to sort 12500 strings, I ran jprof and
found that more than half of this time is used within GetCharsetAtom! I
converted it to using an instance reference to the charset alias service, and
jprof now says GetCharsetAtom takes 2/3 of the time and the sort is down to 2-3s.

Comment 1

[[e-mail]]

Attachment: As advertised

Comment 2

[[e-mail]]

Actually I think there's further room for improvement creating collation keys.

Comment 3

[[e-mail]]

I just got a better idea.

Comment 4

[[e-mail]]

Attachment: Proposed patch

Comment 5

[[e-mail]]

Attachment: diff -w so you can read it

Comment 6

[[e-mail]]

Comment on attachment 120568 [details] [diff] [review]
Proposed patch

This patch makes sorting messages 200% faster on Linux and probably most other
OSes e.g. Win9x but NOT WinNT which already uses fast native unicode sorting.

Comment 7

[Simon Montagu :smontagu]

This looks good, but could we go further? Could GetDefaultCharsetForLocale
return a charset atom instead of a nsString? (If it has to be a string I would
prefer an nsCString, but that's another story.)

Comment 8

[Alec Flett]

Comment on attachment 120569 [details]
diff -w so you can read it

hmm. I have to agree, why don't we just cache the atom?

Comment 9

[[e-mail]]

OK but I didn't want to go mucking around with the interfaces without approval.

Comment 10

[Alec Flett]

approval from whom? :) as long as the interface isn't marked FROZEN (and maybe
UNDER_REVIEW) then the interface is just like any other code - you get the right
module owner review and/or approval and so forth..

Comment 11

[[e-mail]]

Attachment: New approach

OK, so have I got this right?
The only user of nsCollation is nsCollation<platform>.
These never change mCharset after they create nsCollation.
So, they can pass mCharset to a nsCollation method to create the converter.
In fact, I pass the PRUnichar * to save some conversions.
If you forget or fail to set the charset then it falls back on ISO-8859-1.

Comment 12

[[e-mail]]

Comment on attachment 120821 [details] [diff] [review]
New approach

OS/2 never does unicode conversion so I just junked all the charset code.

Comment 13

[[e-mail]]

Oh, and should this get r+sr feel free to check it in before I can on Tuesday.

Comment 14

[Alec Flett]

Comment on attachment 120821 [details] [diff] [review]
New approach

nice. this is a million times better. Sorry for the delay in reading it...

sr=alecf

smontagu, can you r= this  in time for 1.4 beta?

Comment 15

[Simon Montagu :smontagu]

Comment on attachment 120821 [details] [diff] [review]
New approach

r=smontagu

Comment 16

[Simon Montagu :smontagu]

cc-ing mkaply in case there are issues with comment 12.

Comment 17

[Mike Kaply [:mkaply]]

Neil and I talked about this and we think we are OK.

Comment 18

[Simon Montagu :smontagu]

Fix checked in.

Comment 19

[Jan Varga [:janv]]

good work, this may help bug 160140