I was surprised to find that nsLiteral{C,}String has basically the full runtime cost of nsString. I had always assumed that the reason it was created was to let me disguise a string literal as an nsString at no cost. In particular, it's unfortunate that nsLiteralStrings still go through the standard nsAString destructor, even though the flags ensure that we won't actually _do_ anything. And, for global nsLiteralStrings, you have to register that destructor with atexit, so the compiler generates a static constructor that does the registration. I'd like to make nsLiteralString avoid those costs. I tried a few approaches locally, and most of them got real ugly real fast. The best I've come up with is to insert an "even-more-abstract" (ugh) superclass above nsAString, that's little more than a POD struct with the data fields and some const accessors. Then nsLiteralString inherits directly from the POD-thing and avoids getting a destructor. And then we let nsLiteralString implicitly convert to const nsString&. As long as we insist on the const there, the "downcast" should be ok. My local changes save 100k of binary size and a couple dozen static constructors, but I feel kinda dirty about the implementation. Before I spend too much time polishing the patches -- dbaron would you support this approach?

At some point we had a rule that you weren't supposed to use such things in globals. I guess people just forgot about that rule as we grew... The approach seems reasonable assuming we have good enough assertions to enforce it. (Which, in general, we don't -- the string classes need better assertions documenting that we don't break the assumptions of the static type being used, given the way they mix static and dynamic typing.) Although I also wonder what the cost of the *constructors* is, and whether we should instead be trying to avoid global/static nsLiteralString in the first place.

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #1) > Although I also wonder what the cost of the *constructors* is, and whether > we should instead be trying to avoid global/static nsLiteralString in the > first place. In global/static nsLiteralStrings, the constructor exists only for the purpose of registering the destructor with atexit. If you remove the destructor, the constructor disappears entirely; all of the field setup can be done at compile-time.

Comment on attachment 8846132 [details] Bug 1344629 - Part 1: String class cleanup and dead code removal. https://reviewboard.mozilla.org/r/119208/#review121178

Comment on attachment 8846133 [details] Bug 1344629 - Part 2: Add nsTStringRepr as the new root of the string hierarchy. https://reviewboard.mozilla.org/r/119210/#review121180 I'm not crazy about the "Repr" name but I don't have any better ideas right now. r=dbaron with the following comments on comments addressed ::: xpcom/string/nsTSubstring.h:57 (Diff revision 1) > - * nsTSubstring is the most abstract class in the string hierarchy. It > + * nsTStringRepr is the root of the the string hierarchy and defines a string's > + * field layout and some accessors for those fields. This class is intended to > + * be abstract and never instantiated directly. All methods on this class must > + * be const as not all strings are writable. The class is buried in a namespace > + * to discourage its use in function parameters. If you need to take a > + * parameter, use (const) nsA(C)String&. If you need to instantiate a string, > + * use ns(C)String. > + * This comment should explain that the class exists so that nsLiteral[C]String can avoid having a substantive destructor. ::: xpcom/string/nsTSubstring.h:59 (Diff revision 1) > +namespace detail { > + > /** > - * nsTSubstring is the most abstract class in the string hierarchy. It > + * nsTStringRepr is the root of the the string hierarchy and defines a string's > + * field layout and some accessors for those fields. This class is intended to > + * be abstract and never instantiated directly. All methods on this class must I'd also say that it's intended never to be used in any way outside of the string code itself. ::: xpcom/string/nsTSubstring.h:97 (Diff revision 1) > + > +} // namespace detail > +} // namespace mozilla > + > +/** > + * nsTSubstring is an abstract class in the string hierarchy. It I'd also say that as far as the string *API* is concerned, it's the root of the hierarchy.

Comment on attachment 8846134 [details] Bug 1344629 - Part 3: Move const accessors from nsTSubstring to nsTStringRepr. https://reviewboard.mozilla.org/r/119212/#review121186 ::: xpcom/string/nsTSubstring.h:81 (Diff revision 1) > + > typedef nsCharTraits<char_type> char_traits; > typedef char_traits::incompatible_char_type incompatible_char_type; > > - typedef nsTSubstring_CharT self_type; > + typedef nsTStringRepr_CharT self_type; > typedef self_type base_string_type; I wonder if base_string_type should be protected now...

Comment on attachment 8846135 [details] Bug 1344629 - Part 4: Cleanup: make string tuples not think in terms of "substring". https://reviewboard.mozilla.org/r/119214/#review121188

Comment on attachment 8846136 [details] Bug 1344629 - Part 5: Make string tuples work with nsTStringRepr. https://reviewboard.mozilla.org/r/119216/#review121190 ::: xpcom/string/nsTSubstring.h:214 (Diff revision 1) > * equality > */ > > bool NS_FASTCALL Equals(const self_type&) const; > bool NS_FASTCALL Equals(const self_type&, const comparator_type&) const; > + bool NS_FASTCALL Equals(const substring_tuple_type& aTuple) const; Two comments here: (1) Is there a reason you have the implementation in the .cpp file rather than inline in the .h file (which would generate equivalent code to what we used to have). Maybe the codesize is better? I guess it seems OK this way. (2) You should also have an Equals() method that takes a comparator_type, just like the one you've implemented, but wit hthe extra comparator accepted and passed through.

Comment on attachment 8846136 [details] Bug 1344629 - Part 5: Make string tuples work with nsTStringRepr. https://reviewboard.mozilla.org/r/119216/#review121190 > Two comments here: > > (1) Is there a reason you have the implementation in the .cpp file rather than inline in the .h file (which would generate equivalent code to what we used to have). Maybe the codesize is better? I guess it seems OK this way. > > (2) You should also have an Equals() method that takes a comparator_type, just like the one you've implemented, but wit hthe extra comparator accepted and passed through. The implementation needs to call the converting constructor of nsTSubstring, which hasn't yet been defined. Will add the comparator_type.

Comment on attachment 8846137 [details] Bug 1344629 - Part 6: Rewrite unnecessary uses of nsLiteralString. https://reviewboard.mozilla.org/r/119218/#review121194 So I think you're right that this is an antipattern most of the time, and I think most of the changes in this patch are good. But I think this patch goes a little too far. I think we probably do still want get() on nsLiteralString for consistency (but not on Repr -- it should just have its own implementation). I think it does occasionally serve a useful purpose. I think the thing that tipped me against this was the "-1"s that you had to insert; that's one of the things that string classes should save you from, and I think it is not only reasonable, but good, that people use a string class to avoid having to write a -1 after an ArrayLength call. So I think you should in fact land most of these changes as part of this bug. But my inclination is that you should drop: - all the changes where you just changed .get() to Data() - all of the changes that involved a -1 ... and then add .get() to nsLiteral[C]String.

Comment on attachment 8846138 [details] Bug 1344629 - Part 7: Fix up a couple of pointers to literal strings. https://reviewboard.mozilla.org/r/119220/#review121198 ::: dom/media/eme/MediaKeySystemAccess.cpp:346 (Diff revision 1) > { nsCString("audio/webm"), EME_CODEC_VORBIS, MediaDrmProxy::VORBIS, &widevine.mWebM}, > { nsCString("audio/webm"), EME_CODEC_OPUS, MediaDrmProxy::OPUS, &widevine.mWebM}, > }; > > for (const auto& data: validationList) { > - if (MediaDrmProxy::IsCryptoSchemeSupported(kEMEKeySystemWidevine, > + if (MediaDrmProxy::IsCryptoSchemeSupported(kEMEKeySystemWidevine.Data(), Seems like you should find a way to make this work without this change. Seems like there's an existing autoconversion mechanism that this patch is breaking, and it might even be making this code less efficient by making it do a strlen. ::: dom/media/platforms/android/RemoteDataDecoder.cpp:310 (Diff revision 1) > { > JNIEnv* const env = jni::GetEnvForThread(); > > bool formatHasCSD = false; > NS_ENSURE_SUCCESS_VOID( > - aFormat->ContainsKey(NS_LITERAL_STRING("csd-0"), &formatHasCSD)); > + aFormat->ContainsKey(u"csd-0", &formatHasCSD)); same for all of the changes in this file ::: uriloader/exthandler/android/nsExternalSharingAppService.cpp:32 (Diff revision 1) > NS_IMETHODIMP > nsExternalSharingAppService::ShareWithDefault(const nsAString & data, > const nsAString & mime, > const nsAString & title) > { > - NS_NAMED_LITERAL_STRING(sendAction, "android.intent.action.SEND"); > + const char16_t *sendAction = u"android.intent.action.SEND"; and same here ::: widget/android/EventDispatcher.cpp:536 (Diff revision 1) > } > return NS_OK; > } > > nsresult > -UnboxData(jni::String::Param aEvent, JSContext* aCx, jni::Object::Param aData, > +UnboxData(const nsAString& aEvent, JSContext* aCx, jni::Object::Param aData, prefer nsSubstring& to nsAString& Though, actually, I think I'd prefer if somebody more familiar with mozilla::jni::String::Param reviewed this.

Comment on attachment 8846139 [details] Bug 1344629 - Part 8: Make nsTLiteralString inherit from nsTStringRepr. https://reviewboard.mozilla.org/r/119222/#review121202 r=dbaron with the one somewhat-substantive change proposed to the API (though I'm happy to discuss if you disagree) ::: xpcom/string/nsTLiteralString.h:13 (Diff revision 1) > * nsTLiteralString_CharT > * > * Stores a null-terminated, immutable sequence of characters. > * > - * Subclass of nsTString that restricts string value to a literal > - * character sequence. This class does not own its data. The data is > + * nsTString-lookalike that restricts its string value to a literal > + * character sequence. Can be implicitly casted to const nsTString& "casted" -> "cast" ::: xpcom/string/nsTLiteralString.h:15 (Diff revision 1) > * Stores a null-terminated, immutable sequence of characters. > * > - * Subclass of nsTString that restricts string value to a literal > - * character sequence. This class does not own its data. The data is > - * assumed to be permanent. In practice this is true because this code > - * is only usable by and for libxul. > + * nsTString-lookalike that restricts its string value to a literal > + * character sequence. Can be implicitly casted to const nsTString& > + * (the const is essential, since this class's data is not writable). > + * The data is assumed to be static (permanent) and therefore as an "data is" -> "data are" (yes, it was there before, but it's wrong) ::: xpcom/string/nsTLiteralString.h:41 (Diff revision 1) > + const nsTString_CharT* AsString() const > { > + return reinterpret_cast<const nsTString_CharT*>(this); > + } I find it an odd API that AsString() returns a pointer rather than a reference. I'd prefer having the dereference inside of AsString rather than in the type conversion operator, and then have the callers continue to write "&".

Comment on attachment 8846140 [details] Bug 1344629 - Part 9: Bonus fix: Remove heap allocation in nsChromeTreeOwner. https://reviewboard.mozilla.org/r/119224/#review121206

Comment on attachment 8846141 [details] Bug 1344629 - Part 10: Bonus fix: Remove NS_LITERAL_STRING_INIT macros. https://reviewboard.mozilla.org/r/119226/#review121208 Dropping the INIT macros is good. I'm inclined to think it's still good to make everybody write |const| on their literal strings in case we ever change how this works again, and since it generally makes sense since the things are const. So I think I'd prefer to leave the const in the NS_LITERAL_[C]STRING macros. But I'm willing to discuss that if you disagree... r=dbaron with that

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #20) > Seems like you should find a way to make this work without this change. > Seems like there's an existing autoconversion mechanism that this patch is > breaking, and it might even be making this code less efficient by making it > do a strlen. Here's what goes on under the hood: https://dxr.mozilla.org/mozilla-central/rev/528e9dbbb882db0b32792d44b5be9cc539afa1a8/widget/android/jni/Refs.h#746-764 Using a raw pointer doesn't cost any more in terms of allocations, but you're right that it does incur a strlen in the char16_t* case. What do you think about adding another pair of StringParam construtors that take ns(C)LiteralString?

(In reply to David Major [:dmajor] from comment #24) > Here's what goes on under the hood: > https://dxr.mozilla.org/mozilla-central/rev/ > 528e9dbbb882db0b32792d44b5be9cc539afa1a8/widget/android/jni/Refs.h#746-764 > > Using a raw pointer doesn't cost any more in terms of allocations, but > you're right that it does incur a strlen in the char16_t* case. > > What do you think about adding another pair of StringParam construtors that > take ns(C)LiteralString? Sounds good. That's basically what I was hoping you'd do... except I hadn't actually found the code where StringParam was defined since searchfox wasn't being helpful and I was hoping you'd find it for me. :-)

Comment on attachment 8846141 [details] Bug 1344629 - Part 10: Bonus fix: Remove NS_LITERAL_STRING_INIT macros. https://reviewboard.mozilla.org/r/119226/#review121208 My thinking was that the people declaring nsLiteralString in non-macro form aren't getting the const protection anyway. But I don't mind adding the const back.

Thank you for the fast reviews! I'll send out another round on Monday.

Comment on attachment 8846137 [details] Bug 1344629 - Part 6: Rewrite unnecessary uses of nsLiteralString. https://reviewboard.mozilla.org/r/119218/#review121758 ::: layout/generic/nsPageFrame.cpp:194 (Diff revision 2) > - NS_NAMED_LITERAL_STRING(kDate, "&D"); > + const char16_t* kDate = u"&D"; > if (aStr.Find(kDate) != kNotFound) { > - aNewStr.ReplaceSubstring(kDate.get(), mPD->mDateTimeStr.get()); > + aNewStr.ReplaceSubstring(kDate, mPD->mDateTimeStr.get()); > } This change should instead drop the .get() from both parameters, since the ReplaceSubstring being called is going to construct an nsDependentString, so it's better to use the nsLiteralString but without the .get(). Same for the other changes in this file, except the ones that started with only one .get() will likely need an explicit nsDependentString around the second param since.

Comment on attachment 8846780 [details] Bug 1344629 - Part 6.5: Allow get() on nsLiteralStrings, excluding temporaries. https://reviewboard.mozilla.org/r/119780/#review121764

Comment on attachment 8846138 [details] Bug 1344629 - Part 7: Fix up a couple of pointers to literal strings. https://reviewboard.mozilla.org/r/119220/#review121766

Comment on attachment 8846781 [details] Bug 1344629 - Part 7.5: Add constructors for jni::StringParam that accept literal strings. https://reviewboard.mozilla.org/r/119782/#review121770

Pushed by dmajor@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0be052ad24c1 Part 1: String class cleanup and dead code removal. r=dbaron https://hg.mozilla.org/integration/autoland/rev/636095ff2815 Part 2: Add nsTStringRepr as the new root of the string hierarchy. r=dbaron https://hg.mozilla.org/integration/autoland/rev/e5df14c3db61 Part 3: Move const accessors from nsTSubstring to nsTStringRepr. r=dbaron https://hg.mozilla.org/integration/autoland/rev/7d3c06b3eca9 Part 4: Cleanup: make string tuples not think in terms of "substring". r=dbaron https://hg.mozilla.org/integration/autoland/rev/55eee7078ae4 Part 5: Make string tuples work with nsTStringRepr. r=dbaron https://hg.mozilla.org/integration/autoland/rev/53d7d1ce2c97 Part 6: Rewrite unnecessary uses of nsLiteralString. r=dbaron https://hg.mozilla.org/integration/autoland/rev/083304fcd6bd Part 6.5: Allow get() on nsLiteralStrings, excluding temporaries. r=dbaron https://hg.mozilla.org/integration/autoland/rev/0ada91b0452e Part 7: Fix up a couple of pointers to literal strings. r=dbaron https://hg.mozilla.org/integration/autoland/rev/2b460fe020af Part 7.5: Add constructors for jni::StringParam that accept literal strings. r=dbaron https://hg.mozilla.org/integration/autoland/rev/d9b330f9bc24 Part 8: Make nsTLiteralString inherit from nsTStringRepr. r=dbaron https://hg.mozilla.org/integration/autoland/rev/a96390e044e0 Part 9: Bonus fix: Remove heap allocation in nsChromeTreeOwner. r=dbaron https://hg.mozilla.org/integration/autoland/rev/cf4273d3ac30 Part 10: Bonus fix: Remove NS_LITERAL_STRING_INIT macros. r=dbaron

sorry had to back this out for stylo build bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=83575096&repo=autoland&lineNumber=7140 https://hg.mozilla.org/integration/autoland/rev/8d1f842be870

Pushed by dmajor@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/19faa6195bc9 Part 1: String class cleanup and dead code removal. r=dbaron https://hg.mozilla.org/integration/autoland/rev/271be9871274 Part 2: Add nsTStringRepr as the new root of the string hierarchy. r=dbaron https://hg.mozilla.org/integration/autoland/rev/485e1632ef22 Part 3: Move const accessors from nsTSubstring to nsTStringRepr. r=dbaron https://hg.mozilla.org/integration/autoland/rev/e7bf771e8802 Part 4: Cleanup: make string tuples not think in terms of "substring". r=dbaron https://hg.mozilla.org/integration/autoland/rev/62898bbb64d5 Part 5: Make string tuples work with nsTStringRepr. r=dbaron https://hg.mozilla.org/integration/autoland/rev/ac66fac84e17 Part 6: Rewrite unnecessary uses of nsLiteralString. r=dbaron https://hg.mozilla.org/integration/autoland/rev/c3433c49b31d Part 6.5: Allow get() on nsLiteralStrings, excluding temporaries. r=dbaron https://hg.mozilla.org/integration/autoland/rev/06557c34d4e5 Part 7: Fix up a couple of pointers to literal strings. r=dbaron https://hg.mozilla.org/integration/autoland/rev/5504d47acea7 Part 7.5: Add constructors for jni::StringParam that accept literal strings. r=dbaron https://hg.mozilla.org/integration/autoland/rev/c3873cde0276 Part 8: Make nsTLiteralString inherit from nsTStringRepr. r=dbaron https://hg.mozilla.org/integration/autoland/rev/a06239255301 Part 9: Bonus fix: Remove heap allocation in nsChromeTreeOwner. r=dbaron https://hg.mozilla.org/integration/autoland/rev/6066c5ca8f70 Part 10: Bonus fix: Remove NS_LITERAL_STRING_INIT macros. r=dbaron

The stylo side got sorted out thanks to Xidorn. Re-landed the gecko changes as-is.

https://hg.mozilla.org/mozilla-central/rev/19faa6195bc9 https://hg.mozilla.org/mozilla-central/rev/271be9871274 https://hg.mozilla.org/mozilla-central/rev/485e1632ef22 https://hg.mozilla.org/mozilla-central/rev/e7bf771e8802 https://hg.mozilla.org/mozilla-central/rev/62898bbb64d5 https://hg.mozilla.org/mozilla-central/rev/ac66fac84e17 https://hg.mozilla.org/mozilla-central/rev/c3433c49b31d https://hg.mozilla.org/mozilla-central/rev/06557c34d4e5 https://hg.mozilla.org/mozilla-central/rev/5504d47acea7 https://hg.mozilla.org/mozilla-central/rev/c3873cde0276 https://hg.mozilla.org/mozilla-central/rev/a06239255301 https://hg.mozilla.org/mozilla-central/rev/6066c5ca8f70