387023 - Minor string cleanup in nsAtomTable.cpp

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Eli Friedman]

Attachment: Patch

Avoid a bit of potential unnecessary string copying and make nsStaticAtomWrapper hold onto its string directly.

Comment 1

[Eli Friedman]

Comment on attachment 271117 [details] [diff] [review]
Patch

There's more simplification that really needs to be done; the PromiseFloatString call in NS_NewAtom actually seems to take up a non-trivial amount of time.

Comment 2

[Eli Friedman]

Attachment: Patch v2

Some small stuff, could lead to a minor perf improvement by avoiding calling PromiseFlatString and avoiding string copies in some cases.

Comment 3

[Eli Friedman]

Comment on attachment 278119 [details] [diff] [review]
Patch v2

Oops, wrong version.

Comment 4

[Eli Friedman]

Attachment: Patch v3

All right, here's the right version.

Comment 5

[Benjamin Smedberg]

Comment on attachment 278120 [details] [diff] [review]
Patch v3

>Index: nsAtomTable.cpp

>-      CompareUTF8toUTF16(nsDependentCString(atomString, he->getLength()),
>-                         nsDependentString(strKey->getUTF16String(),
>-                                           strKey->getLength())) == 0;
>+      CompareUTF8toUTF16(nsDependentCSubstring(atomString, atomString + he->getLength()),
>+                         nsDependentSubstring(strKey->getUTF16String(),
>+                                              strKey->getUTF16String() + strKey->getLength())) == 0;

I understand all the changes except for this one. Why did you need to change to the substring type here?

Comment 6

[Eli Friedman]

I don't think it's legal to use nsDependentString on a buffer that isn't guaranteed to be null-terminated.  I don't think it actually matters here, though, because CompareUTF8toUTF16 doesn't happen to care.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 278120 [details] [diff] [review]
Patch v3

a=bzbarsky assuming he .NET thing still compiles

Comment 8

[Eli Friedman]

Checked in.