Closed
Bug 207634
Opened 22 years ago
Closed 21 years ago
Want AllocateRawSortKey function
Categories
(Core :: Internationalization: Localization, enhancement, P4)
Tracking
()
RESOLVED
FIXED
Future
People
(Reporter: neil, Assigned: smontagu)
References
Details
(Keywords: perf)
Attachments
(1 file, 7 obsolete files)
53.15 KB,
patch
|
smontagu
:
review+
sspitzer
:
superreview+
|
Details | Diff | Splinter Review |
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•22 years ago
|
||
With this patch sorting 13270 messages by subject dropped from 1.414745s to 1.07754s (single test).
Reporter | ||
Comment 3•22 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 5•22 years ago
|
||
My apologies to anyone looking for the about:config filtering bug 207840
Reporter | ||
Updated•22 years ago
|
Attachment #125749 -
Flags: review?(smontagu)
Assignee | ||
Comment 7•22 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•22 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•22 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•22 years ago
|
||
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•22 years ago
|
||
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•22 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•22 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•22 years ago
|
||
Thanks to kaie@netscape.com for testing this on the Mac.
Attachment #126072 -
Attachment is obsolete: true
Attachment #126122 -
Attachment is obsolete: true
Comment 15•22 years ago
|
||
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•22 years ago
|
Priority: -- → P4
Updated•22 years ago
|
Target Milestone: --- → Future
Reporter | ||
Comment 18•21 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 19•21 years ago
|
||
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•21 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•21 years ago
|
||
Comment on attachment 127179 [details] [diff] [review] Patch with OS2 fixes r=smontagu
Attachment #127179 -
Flags: review?(smontagu) → review+
Reporter | ||
Comment 23•21 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
Closed: 21 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•21 years ago
|
Attachment #125749 -
Flags: review?(smontagu)
You need to log in
before you can comment on or make changes to this bug.
Description
•