Quoth the XPCOM string guide https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings : "Another common incorrect pattern is to use nsAutoString/nsAutoCString for member variables. As described in Local Variables, these classes have a built in buffer that make them very large. This means that if you include them in a class, they bloat the class by 64 bytes (nsAutoCString) or 128 bytes (nsAutoString)." We should write a static analysis to disallow these.

This obviously breaks the build, and I'll attach one or several patches to remove all consequent static analysis failures once I have green trys.

(In reply to Mike Hommey [:glandium] from comment #1) > Created attachment 8688847 [details] [diff] [review] > Only allow nsAuto{,C}String on the stack > > This obviously breaks the build, and I'll attach one or several patches to > remove all consequent static analysis failures once I have green trys. This patch doesn't act identically to the requested analysis, as stack-allocated structs which have nsAuto{,C}Strings in them are still allowed. However, I would think that the thing which we are trying to avoid is the heap allocated ones (as allocating a nsAuto{,C}String in a struct on the stack is mostly equivalent to not-in-a-struct on the stack), so this patch should be good. I just wanted to make sure that the difference was clear :).

Comment on attachment 8688847 [details] [diff] [review] Only allow nsAuto{,C}String on the stack Review of attachment 8688847 [details] [diff] [review]: ----------------------------------------------------------------- Sure.

Comment on attachment 8689765 [details] [diff] [review] Don't use nsAuto{,C}String as class member variables in docshell/ r=me

Comment on attachment 8689777 [details] [diff] [review] Don't use nsAuto{,C}String as class member variables in security/manager/ Review of attachment 8689777 [details] [diff] [review]: ----------------------------------------------------------------- Sounds good.

I don't want us to prevent nsAuto*String as a member variable. It can be very useful for performance. Or at least we need to have some way to have pre-allocated strings. Perhaps some other name for such strings?

Comment on attachment 8689763 [details] [diff] [review] Don't use nsAuto{,C}String as class member variables in dom/ StringBuilder stuff in nsContentUtils is super performance critical (in some silly microbenchmarks). Because of that, r-

(In reply to Olli Pettay [:smaug] from comment #22) > Comment on attachment 8689763 [details] [diff] [review] > Don't use nsAuto{,C}String as class member variables in dom/ > > StringBuilder stuff in nsContentUtils is super performance critical > (in some silly microbenchmarks). > Because of that, r- Where is the microbenchmark?

https://bugzilla.mozilla.org/show_bug.cgi?id=744830#c51 is one case.

Comment on attachment 8689780 [details] [diff] [review] Don't use nsAuto{,C}String as class member variables in uriloader/ This one is weird. Yes, nsAutoCString is 64 bytes long... but that's exactly the length of an SHA-256 hash. I guess this is not super-critical, so r=me

The fun part is that StringBuilder stuff all has stack lifetime. Just not obviously so from inside the class. So using an autostring there is not unreasonable.

Comment on attachment 8689770 [details] [diff] [review] Don't use nsAuto{,C}String as class member variables in layout/ I still need to look at the rest of this, but the use of nsAutoString in the CSS scanner is very purposeful. We don't want to pay the cost of heap allocations in the common case there.

(In reply to Boris Zbarsky [:bz] from comment #25) > Comment on attachment 8689780 [details] [diff] [review] > Don't use nsAuto{,C}String as class member variables in uriloader/ > > This one is weird. Yes, nsAutoCString is 64 bytes long... but that's > exactly the length of an SHA-256 hash. Actually, a SHA-256 is 32 bytes. And since it's fixed size, it could be a char[32] instead of also having all the nsString things like flags, length, and pointer to the actual string...

But I guess it's a sha-256 in hex form, which makes it indeed a 64 bytes string. The comment still applies, though.

Comment on attachment 8689770 [details] [diff] [review] Don't use nsAuto{,C}String as class member variables in layout/ >+++ b/layout/base/nsLayoutUtils.h So most of the uses of nsSetAttrRunnable are in fact passing in an integer aValue, so we'll get an extra allocation. And being runnables these are transient.... I guess I can live with this, but doing something with an auto buffer here actually does make sense. >+++ b/layout/style/nsCSSScanner.h This change I expect to be bad for performance. I would rather not do this. r- for this part.

Comment on attachment 8689769 [details] [diff] [review] Don't use nsAuto{,C}String as class member variables in image/ Review of attachment 8689769 [details] [diff] [review]: ----------------------------------------------------------------- In both of these cases nsAutoCString was intentionally used to avoid extra dynamic allocations in the extremely common case that the strings involved are small. I think in both of these cases, we can live without that optimization, since ImageURL is quite long-lived and the strings in NewPartResult end up getting assigned elsewhere, presumably sharing the same dynamically allocated buffer. However, I'm not convinced that it's never appropriate to use nsAutoCString as a member variable type, because dynamic allocation has costs and it's frequently the case that strings are small, so I question the premise of this bug.

I guess the premise is to avoid a footgun, e.g. accidentally bloating a struct with these members. But if it's a conscious decision (e.g. there won't be many of these structs live at any given time, and performance is crucial), perhaps an annotation would be useful (something like MOZ_INPLACE_BUFFER)? (I was going to suggest MOZ_LARGE_MEMBER, but that has unfortunate connotations)

(In reply to Seth Fowler [:seth] [:s2h] from comment #31) > Comment on attachment 8689769 [details] [diff] [review] > Don't use nsAuto{,C}String as class member variables in image/ > > Review of attachment 8689769 [details] [diff] [review]: > ----------------------------------------------------------------- > > In both of these cases nsAutoCString was intentionally used to avoid extra > dynamic allocations in the extremely common case that the strings involved > are small. I think in both of these cases, we can live without that > optimization, since ImageURL is quite long-lived and the strings in > NewPartResult end up getting assigned elsewhere, presumably sharing the same > dynamically allocated buffer. However, I'm not convinced that it's never > appropriate to use nsAutoCString as a member variable type, because dynamic > allocation has costs and it's frequently the case that strings are small, so > I question the premise of this bug. Look around for the uses of nsAuto*Strings. There are many cases where it's obvious they were used because of the Auto name, and not because of what they do. It's even more obvious when you see it used for function arguments and *return values*. Now, there appears to be a few cases where it might actually be desirable, but maybe that's not entirely true either. In the image/ cases, for things like schemes or content types, I don't see why not use something like Atoms (that might be true of the CSSScanner too). In the StringBuilder case, I don't see much reason not to use plain char16_t pointers, although that would require some minor manual handling.

> I don't see why not use something like Atoms (that might be true of the CSSScanner too) What the CSS scanner uses this string for is an accumulation buffer for the current token. It needs to be fast, and it needs to be able to change value without thrashing the allocator. Using an atom (assuming you meant nsIAtom) would not work at all.

Comment on attachment 8689779 [details] [diff] [review] Don't use nsAuto{,C}String as class member variables in toolkit/ Review of attachment 8689779 [details] [diff] [review]: ----------------------------------------------------------------- None of these appear to be performance-critical.

Comment on attachment 8689784 [details] [diff] [review] Don't use nsAuto{,C}String as class member variables in xpcom/ Review of attachment 8689784 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the changes below. ::: xpcom/reflect/xptinfo/ShimInterfaceInfo.h @@ +42,5 @@ > ~ShimInterfaceInfo() {} > > private: > nsIID mIID; > + nsCString mName; We could profitably turn this into a |const char*|, since it only ever refers to names from kComponentsInterfaceShimMap, and those names are statically allocated.

Comment on attachment 8689768 [details] [diff] [review] Don't use nsAuto{,C}String as class member variables in gfx/ Review of attachment 8689768 [details] [diff] [review]: ----------------------------------------------------------------- Sure.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5d653011477 https://hg.mozilla.org/integration/mozilla-inbound/rev/dfc8f8660578 https://hg.mozilla.org/integration/mozilla-inbound/rev/2c6b93ef7ace https://hg.mozilla.org/integration/mozilla-inbound/rev/45d6cad904db https://hg.mozilla.org/integration/mozilla-inbound/rev/9a929584adeb https://hg.mozilla.org/integration/mozilla-inbound/rev/958002a310a3 https://hg.mozilla.org/integration/mozilla-inbound/rev/210627948092 https://hg.mozilla.org/integration/mozilla-inbound/rev/15bbe7c7bb3c https://hg.mozilla.org/integration/mozilla-inbound/rev/fc1d53003036 https://hg.mozilla.org/integration/mozilla-inbound/rev/bb12e45c0dd8

(In reply to Boris Zbarsky [:bz] from comment #25) > Comment on attachment 8689780 [details] [diff] [review] > Don't use nsAuto{,C}String as class member variables in uriloader/ > > This one is weird. Yes, nsAutoCString is 64 bytes long... but that's > exactly the length of an SHA-256 hash. Actually... not. First, nsIBackgroundFileSaver.idl says that sha256Hash is the SHA-256 hash, in raw bytes. So that's 32 bytes. But it's an ACString, so it still has an additional null terminator, which makes it a 33 bytes string. But wait for the best part: what BackgroundFileSaver::GetSha256Hash does is assign to the out ACString with operator =, and here is what can be seen after the return from the call to GetSha256Hash from nsExternalAppHandler::OnSaveComplete in gdb: (gdb) print mHash $1 = {<nsFixedCString> = {<nsCString> = {<nsACString_internal> = { mData = 0x7fffc40481c8 "\177\351\340G*\227\070ɔ\262Y\021\242\365U\370\322\377F\347'tn\346\355\202\354lk\004\210\061", mLength = 32, mFlags = 65541}, <No data fields>}, mFixedCapacity = 63, mFixedBuf = 0x7fffccf70378 ""}, mStorage = "\000", '\345' <repeats 62 times>, <incomplete sequence \345>} Yup, nsAutoString's inline storage is not even used. I wouldn't be surprised if in the other cases where "performance matter", the same is also happening (I will check).

Ah, this thing is created as a copy of another string in all cases? In that case, it only makes sense to make it an autostring if that other string is an autostring using its inline storage. I'm 99.9% sure this is NOT the scenario in the CSS scanner. ;)

(In reply to Boris Zbarsky [:bz] from comment #40) > I'm 99.9% sure this is NOT the scenario in the CSS scanner. ;) Evidence says this is true. However, the story is different for the changes in nsContentUtils.cpp that smaug r-ed.

Comment on attachment 8689763 [details] [diff] [review] Don't use nsAuto{,C}String as class member variables in dom/ Review of attachment 8689763 [details] [diff] [review]: ----------------------------------------------------------------- Evidence shows that on the microbenchmark mentioned in comment 24, of 1,334,797 nsAutoStrings of size <= 63 used for StringBuilder::Unit::mString, 1,274,797 (95.5%) do *NOT* use the inline buffer.

https://hg.mozilla.org/mozilla-central/rev/a5d653011477 https://hg.mozilla.org/mozilla-central/rev/dfc8f8660578 https://hg.mozilla.org/mozilla-central/rev/2c6b93ef7ace https://hg.mozilla.org/mozilla-central/rev/45d6cad904db https://hg.mozilla.org/mozilla-central/rev/9a929584adeb https://hg.mozilla.org/mozilla-central/rev/958002a310a3 https://hg.mozilla.org/mozilla-central/rev/210627948092 https://hg.mozilla.org/mozilla-central/rev/15bbe7c7bb3c https://hg.mozilla.org/mozilla-central/rev/fc1d53003036 https://hg.mozilla.org/mozilla-central/rev/bb12e45c0dd8

Comment on attachment 8689763 [details] [diff] [review] Don't use nsAuto{,C}String as class member variables in dom/ I still don't quite understand why to change StringBuilder, but fine. (I'll upload a case where the performance does regress because we end up doing more malloc/free)

When looking at GetAttr of this kind of silly benchmark, that does show quite a bit more allocations. But this is a super artificial test.

(In reply to Olli Pettay [:smaug] from comment #46) > Created attachment 8694905 [details] > innerhtml test > > When looking at GetAttr of this kind of silly benchmark, that does show > quite a bit more allocations. But this is a super artificial test. Indeed, this testcase does use the inline storage for all the 3,440,640 uses of nsAutoString from the StringBuilder. (In reply to Olli Pettay [:smaug] from comment #45) > Comment on attachment 8689763 [details] [diff] [review] > Don't use nsAuto{,C}String as class member variables in dom/ > > I still don't quite understand why to change StringBuilder. Because if there are only 2 use cases in the entire tree where using nsAutoString for a member variable /might/ be worth (which in practice is not even a given), we might as well figure how we can do without for those 2. And the thing is, for StringBuilder, we could probably even do without an nsString at all, and that would do even less allocations. I'll try that.

With the code currently in the tree: https://bug-81214-attachments.webkit.org/attachment.cgi?id=132034 foo div.innerHTML : 1.2ms foo div.outerHTML : 1.2ms <div id="node">foo</div> body.innerHTML : 209.4ms body.outerHTML : 212ms https://bugzilla.mozilla.org/attachment.cgi?id=8694905 Running 20 times Ignoring warm-up run (87) 86 88 86 85 85 86 86 87 86 86 86 86 87 87 113 87 86 87 86 85 avg 87.55 stdev 5.886212704277684 With the previous patch (using nsString*): https://bug-81214-attachments.webkit.org/attachment.cgi?id=132034 foo div.innerHTML : 1.2ms foo div.outerHTML : 1.2ms <div id="node">foo</div> body.innerHTML : 209ms body.outerHTML : 213ms https://bugzilla.mozilla.org/attachment.cgi?id=8694905 Running 20 times Ignoring warm-up run (102) 101 104 100 99 101 100 100 99 99 102 100 101 100 100 103 130 100 100 100 99 avg 101.9 stdev 6.5719099202591025 With this new patch: https://bug-81214-attachments.webkit.org/attachment.cgi?id=132034 foo div.innerHTML : 1.2ms foo div.outerHTML : 1.2ms <div id="node">foo</div> body.innerHTML : 208.4ms body.outerHTML : 213.8ms https://bugzilla.mozilla.org/attachment.cgi?id=8694905 Running 20 times Ignoring warm-up run (84) 84 86 84 84 84 85 83 84 85 85 84 84 85 86 111 84 84 84 84 86 avg 85.8 stdev 5.8360945845659495 The number of allocations should be unchanged, or reduced for strings longer than 63 characters, and the size of allocations should be reduced overall.

Note that considering the noise in the test results when running them multiple times, the only really conclusive thing is that https://bugzilla.mozilla.org/attachment.cgi?id=8694905 *was* slower with the nsString* variant, and that the new patch has the same performance characteristics as the code currently in the tree.

Comment on attachment 8694983 [details] [diff] [review] Don't use nsAuto{,C}String as class member variables in dom/ > void Append(const nsAString& aString) > { > Unit* u = AddUnit(); >- u->mString = new nsAutoString(aString); >+ u->mString = NS_strndup(aString.Data(), aString.Length()); you don't want this, since usually we just share the string data when mString is ns*String

(In reply to Olli Pettay [:smaug] from comment #50) > >- u->mString = new nsAutoString(aString); > >+ u->mString = NS_strndup(aString.Data(), aString.Length()); > you don't want this, since usually we just share the string data when > mString is ns*String mString = new nsAutoString(aString); only shares the string data if aString has the F_SHARED flag, and in that case a nsAutoString gives no advantage, quite the contrary. I don't know if the strings passed in would have that flag. Now, looking at the callers, this code is only used for: DOCUMENT_TYPE_NODE, PROCESSING_INSTRUCTION_NODE, and element nodes that are neither HTML, SVG or MathML. So in practice, this code is never called in neither of the test case you gave me, so I can't tell for sure.

Some digging tells me those nsStrings are coming from StringBuffer::ToString, which uses shared strings.

I accidentally added a new nsAutoString as a member variable in bug 1240118 - this should fix it (luckily it's for a singleton class, but still).

Pushed by dkeeler@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ffb38910dcae (still) don't use nsAutoString as class member variable in security/manager/ r=mgoodwin

The leave-open keyword is there and there is no activity for 6 months. :glandium, maybe it's time to close this bug?