Want AllocateRawSortKey function

RESOLVED FIXED in Future

Status

()

P4
enhancement
RESOLVED FIXED
16 years ago
11 years ago

People

(Reporter: neil, Assigned: smontagu)

Tracking

({perf})

Trunk
Future
x86
Windows 2000
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

16 years ago
All callers of CreateRawSortKey call GetRawSortKeyLen first, allocate the key,
then create the raw sort key. Since these two functions share code it should be
more efficient to replace or supplement these with a new AllocateRawSortKey (or
whatever naming convention) function.
(Reporter)

Comment 1

16 years ago
Created attachment 125229 [details] [diff] [review]
First go at a patch

With this patch sorting 13270 messages by subject dropped from 1.414745s to
1.07754s (single test).
(Reporter)

Comment 2

16 years ago
Created attachment 125512 [details] [diff] [review]
Nearly there!
Attachment #125229 - Attachment is obsolete: true
(Reporter)

Comment 3

16 years ago
I don't understand the nsCollationMacUC code though, I can't figure out the
parameters to UCGetCollationKey so that I can allocate a collation key.
(Reporter)

Comment 4

16 years ago
Created attachment 125586 [details] [diff] [review]
Improved CompareRawSortKey
Attachment #125512 - Attachment is obsolete: true
(Reporter)

Comment 5

16 years ago
My apologies to anyone looking for the about:config filtering bug 207840
(Reporter)

Comment 6

16 years ago
Created attachment 125749 [details] [diff] [review]
Ready for testing
Attachment #125586 - Attachment is obsolete: true
(Reporter)

Updated

16 years ago
Attachment #125749 - Flags: review?(smontagu)
(Assignee)

Comment 7

16 years ago
Comment on attachment 125749 [details] [diff] [review]
Ready for testing

There's something funny going on here:

>+nsresult nsCollactionMac::CompareString()
>+{
>+
>+nsresult nsCollationMac::CompareString(const nsCollationStrength strength, 
>+                                       const nsAString& string1, const nsAString& string2, 

more substantive comments to follow...
(Assignee)

Comment 8

16 years ago
>-  // order could be -2, -1, 0, 1, or 2, 
>-  // *result can only be -1, 0 or 1
>-  *result = (order == 0) ? 0 : ((order > 0) ? 1 : -1);

Is it safe to remove this? Do callers only depend on the result being <0 / ==0 /
>0 ?

>+  // set our default collaion options

Nit: might as well fix this typo if you will have cvsblame for the line anyway

You will need mac- and OS/2-specific review and pre-checkin testing, but I am
very happy with this overall.
(Assignee)

Comment 9

16 years ago
Comment on attachment 125749 [details] [diff] [review]
Ready for testing

I discovered a few typos trying to build this on Windows:

>+    if (retval) {
>+      res = NS_ERROR_OK;

s/ERROR_//


>+      if (NS_SUCCEEDED(res) && Cstr2 != nsnull) {
>+        retval = CompareStringA(mLCID, dwMapFlags, Cstr1, -1, CStr2, -1);

s/CStr2/Cstr2/

>+    buffer = PR_Malloc(len);

s/len/byteLen/
(two of these)

Also, defining CompareString in nsCollationWin.cpp collides with a Windows (and
I think also a Mac) #define. Using the same name is silly IMNSHO, and has
caused bustage before now
(c/nsBookmarksService.cpp#1.288">http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/xpfe/components/bookmarks/sr c/nsBookmarksService.cpp#1.288)
Maybe it would be a good idea to change the name?

Just to get it to compile for now I added #undef CompareString after the
#include <windows.h> in nsCollationWin.cpp. Now testing...
(Assignee)

Comment 10

16 years ago
Created attachment 126072 [details]
Stack trace

Running with this on Windows, when I open mailnews I get 
###!!! ASSERTION: Should only be initialized once.: 'mCollation == NULL', file
c:/MozillaWork/shaping1/mozilla/intl/locale/src/windows/nsCollationWin.cpp,
line 70

followed by a crash with this stack
(Reporter)

Comment 11

16 years ago
Created attachment 126122 [details] [diff] [review]
Updated patch

Perhaps your error was caused by an incomplete build - I had forgotton to diff
nsCharsetMenu.cpp (and nsFontMetricsOS2.cpp). This patch should also
incorporate your previous comments.
Attachment #125749 - Attachment is obsolete: true
(Assignee)

Comment 12

16 years ago
Comment on attachment 126122 [details] [diff] [review]
Updated patch

OK, this one builds out of the box and sorts my mail without crashing (after I
built from toplevel, which I should probably have done in the first place --
sorry!)

Comment 13

16 years ago
I'm not sure I understand what this bug is about, but...

Neil gave me a patch, it compiled on a 1.4 branch Mac OSX tree from 2nd of June,
and we tried "sort by subject" order in mail window. I saw that "Re: " and case
were ignored in terms of sort order, but besides that the sort order looked fine.
(Reporter)

Comment 14

16 years ago
Created attachment 126562 [details] [diff] [review]
Patch with Mac fixes

Thanks to kaie@netscape.com for testing this on the Mac.
Attachment #126072 - Attachment is obsolete: true
Attachment #126122 - Attachment is obsolete: true
Very strange. We crash on startup on OS/2.

For some reason, the UniStrlen on the Unistrxfrmed string is very large.

We're still investigating.

Updated

16 years ago
Priority: -- → P4

Updated

16 years ago
Target Milestone: --- → Future
(Reporter)

Comment 16

16 years ago
Created attachment 127179 [details] [diff] [review]
Patch with OS2 fixes
Attachment #126562 - Attachment is obsolete: true
This should be good for OS/2.
(Reporter)

Comment 18

16 years ago
Comment on attachment 127179 [details] [diff] [review]
Patch with OS2 fixes

Asking for Seth's sr because of minor mailnews impact.
Attachment #127179 - Flags: superreview?(sspitzer)
Attachment #127179 - Flags: review?(shafalus)
Comment on attachment 127179 [details] [diff] [review]
Patch with OS2 fixes

r/sr=sspitzer on the mailnews parts.

just a few questions:

1)

-  if (abcard->primaryCollationKey)
-    nsMemory::Free(abcard->primaryCollationKey);
-  if (abcard->secondaryCollationKey)
-    nsMemory::Free(abcard->secondaryCollationKey);
+  PR_FREEIF(abcard->primaryCollationKey);
+  PR_FREEIF(abcard->secondaryCollationKey);
   PR_FREEIF(abcard);

Is PR_Free() the right thing?  Does it match the allocator?  (We don't want any
mis-matched memory frees)


2)

   // XXX can we avoid this copy?
   nsAutoString sourceString(aSource);
-  rv = mCollationKeyGenerator->GetSortKeyLen(kCollationCaseInSensitive,
sourceString, aKeyLen);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  *aKey = (PRUint8*) nsMemory::Alloc(*aKeyLen);
-  if (!aKey)
-    return NS_ERROR_OUT_OF_MEMORY;
-
-  rv = mCollationKeyGenerator->CreateRawSortKey(kCollationCaseInSensitive,
sourceString, *aKey, aKeyLen);
+  rv = mCollationKeyGenerator->AllocateRawSortKey(kCollationCaseInSensitive,
sourceString, aKey, aKeyLen);

Can we remove that nsAutoString now?
Attachment #127179 - Flags: superreview?(sspitzer) → superreview+
(Reporter)

Comment 21

16 years ago
1) AllocateRawSortKey uses PR_Malloc, so all the callers have to use PR_Free
2) It looks as if nsDependentString(aSource) should always have worked anyway.
I'll try to remember to test and roll it into the patch.
(Assignee)

Comment 22

16 years ago
Comment on attachment 127179 [details] [diff] [review]
Patch with OS2 fixes

r=smontagu
Attachment #127179 - Flags: review?(smontagu) → review+
(Reporter)

Comment 23

16 years ago
Patch is in... well, attachment 127179 [details] [diff] [review] was missing the mac fixes :-[
plus also thanks to mkaply for spotting the missing #include "nsCRT.h"
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Reporter)

Comment 24

15 years ago
*** Bug 207512 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 25

15 years ago
*** Bug 99383 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 26

15 years ago
*** Bug 195008 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

15 years ago
Attachment #125749 - Flags: review?(smontagu)
(Reporter)

Updated

11 years ago
Duplicate of this bug: 103451
You need to log in before you can comment on or make changes to this bug.