1265138 - Change nsDependentStrings in nsStaticCaseInsensitiveNameTable to const char*

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

nsStaticCaseInsensitiveNameTable takes array of static C strings and wraps
them in a heap-allocated array of nsDependentCString. This patches removes that
wrapping, avoiding 22 KiB (on 64-bit) of heap allocations per process.

This change also simplifies things slightly overall, though it does change the
return type of a few functions, mostly in layout. The callsites of those
functions feature a mix of ASCII and UTF8 conversions, so the patch also makes
them all ASCII for consistency.

MozReview-Commit-ID: GjijeSV5s4B

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Change nsDependentStrings in nsStaticCaseInsensitiveNameTable to const char*

nsStaticCaseInsensitiveNameTable takes array of static C strings and wraps
them in a heap-allocated array of nsDependentCString. This patches removes that
wrapping, avoiding 22 KiB (on 64-bit) of heap allocations per process.

This change also simplifies things slightly overall, though it does change the
return type of a few functions, mostly in layout. The callsites of those
functions feature a mix of ASCII and UTF8 conversions, so the patch also makes
them all ASCII for consistency.

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 8742039 [details] [diff] [review]
Change nsDependentStrings in nsStaticCaseInsensitiveNameTable to const char*

Review of attachment 8742039 [details] [diff] [review]:
-----------------------------------------------------------------

erahm: please review the xpcom/ parts.

heycam: please review the other parts.

Comment 3

[Cameron McCormack (:heycam)]

Comment on attachment 8742039 [details] [diff] [review]
Change nsDependentStrings in nsStaticCaseInsensitiveNameTable to const char*

Review of attachment 8742039 [details] [diff] [review]:
-----------------------------------------------------------------

It seems like a shame to move away from string classes to dealing with raw char* pointers.  Currently, we compute the length of these strings all ahead of time, due to wrapping them in nsDependentCStrings.  With these patches, when we explicitly wrap these char* pointers in an nsDependentCString, or pass them to e.g. NS_ConvertASCIItoUTF16, we will compute the lengths afresh each time.  I wonder if saving 22 KiB per process is worth the awkwardness of using raw char* pointers, and the slower interactions with these strings.

Since we know the lengths of these strings at compile time, I wonder if a good compromise would be to pass in an array of the char*s and their lengths (both static data) to the nsStaticCaseInsensitiveNameTable, and create nsDependentCStrings for return values on demand using that information?

It's very likely that the names of CSS properties, font descriptors, counter descriptors and keywords will never be anything other than ASCII.  But to be safe, can we add some assertions (maybe in CreateStaticTable in nsCSSProps.cpp) that all the strings we add to these tables are indeed ASCII strings?