160140 - sort in xul treecol does not use locale sensitive sorting

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Frank Tang]

this is spin off from 159188. in 159188 the sort order in treecol does not sort
according to the locale order. 
naoki, please work with xul tree owner to fix it and use the sort services

Comment 1

[Jan Varga [:janv]]

mine

Comment 2

[Jan Varga [:janv]]

Attachment: proposes fix

This was probably not correctly copied from the XUL sort service.
I spent some time until I found out it's actually a copy/paste mistake.
Now I know how it works at least :)

Comment 3

[Jan Varga [:janv]]

sigh, this will have noticable performance impact

Comment 4

[Jan Varga [:janv]]

Comment on attachment 115087 [details] [diff] [review]
proposes fix

I've changed my mind, CompareString is expensive, we need to cache sort keys
somehow.

Comment 5

[Jan Varga [:janv]]

I tried to sort a data source with more than 1000 rows (not so big).
Sorting of this data source was noticeable slower, even on my 1.5 Ghz machine.
CompareString has to call GetRawSortKeyLen and CreateRawSortKey every time.
So we end up with many redundant calls to GetRawSortKeyLen and CreateRawSortKey
for the same string. 
I think we can "pre-cache" these sort keys using an arena and then call
CompareRawSortKey directly (as we do now).
Chris Waterson says it's a good idea.

Comment 6

[Jan Varga [:janv]]

Attachment: patch

Fix for bug 201670 helped sorting performance a lot (around 300% according to
Neil)
I think we should land this for correctness since XUL sort service use the same
API. I'll also file a new bug about caching sort keys to improve performance
even more.

Comment 7

[[e-mail]]

Comment on attachment 121268 [details] [diff] [review]
patch

>+                                              nsDependentString(lstr),
>+                                              nsDependentString(rstr),
>+                                              &result);
>                 }
>                 else
>                     result = ::Compare(nsDependentString(lstr),
>                                        nsDependentString(rstr),
Worth sharing those nsDependentStrings?

Comment 8

[jag (Peter Annema)]

Simplified, the code is:

if (foo) {
  use1(nsDependentString(foopy));
} else {
  use2(nsDependentString(foopy));
}

When run through, only one nsDependentString will be created, so there's no
sharing possible, really. The compiler should even be able to reorder the code
so it'll only generate the constructor (call) asm once, no need for us to do
that IMO.

Comment 9

[jag (Peter Annema)]

Comment on attachment 121268 [details] [diff] [review]
patch

sr=jag (didn't I sr a patch for this a month ago?)

Comment 10

[Jan Varga [:janv]]

Checked in.
Bug 203007 filed for the perf issue.