disallow member variables of type nsAuto{C,}String
Categories
(Developer Infrastructure :: Source Code Analysis, defect, P3)
Tracking
(Not tracked)
People
(Reporter: froydnj, Assigned: glandium)
References
(Blocks 1 open bug)
Details
(Keywords: leave-open, Whiteboard: [MemShrink:P3])
Attachments
(18 files, 2 obsolete files)
1.05 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
14.67 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
3.42 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
1.52 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
1.16 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
1.86 KB,
patch
|
seth
:
review+
|
Details | Diff | Splinter Review |
11.30 KB,
patch
|
bzbarsky
:
review-
|
Details | Diff | Splinter Review |
3.10 KB,
patch
|
mcmanus
:
review+
|
Details | Diff | Splinter Review |
1006 bytes,
patch
|
Pike
:
review+
|
Details | Diff | Splinter Review |
1.74 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
4.90 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
1.26 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
2.61 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
1018 bytes,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
3.60 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
1.68 KB,
text/html
|
Details | |
18.45 KB,
patch
|
Details | Diff | Splinter Review | |
1.00 KB,
patch
|
mgoodwin
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 1•8 years ago
|
||
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.
Comment 2•8 years ago
|
||
(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 :).
![]() |
Reporter | |
Comment 3•8 years ago
|
||
Comment on attachment 8688847 [details] [diff] [review] Only allow nsAuto{,C}String on the stack Review of attachment 8688847 [details] [diff] [review]: ----------------------------------------------------------------- Sure.
Assignee | ||
Comment 4•8 years ago
|
||
Assignee | ||
Comment 5•8 years ago
|
||
Assignee | ||
Comment 6•8 years ago
|
||
Assignee | ||
Comment 7•8 years ago
|
||
![]() |
||
Comment 8•8 years ago
|
||
Comment on attachment 8689765 [details] [diff] [review] Don't use nsAuto{,C}String as class member variables in docshell/ r=me
Assignee | ||
Comment 9•8 years ago
|
||
Assignee | ||
Comment 10•8 years ago
|
||
Assignee | ||
Comment 11•8 years ago
|
||
Assignee | ||
Comment 12•8 years ago
|
||
Assignee | ||
Comment 13•8 years ago
|
||
Updated•8 years ago
|
Assignee | ||
Comment 14•8 years ago
|
||
Assignee | ||
Comment 15•8 years ago
|
||
Assignee | ||
Comment 16•8 years ago
|
||
Assignee | ||
Comment 17•8 years ago
|
||
Assignee | ||
Comment 18•8 years ago
|
||
Assignee | ||
Comment 19•8 years ago
|
||
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.
Updated•8 years ago
|
Comment 21•8 years ago
|
||
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 22•8 years ago
|
||
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-
Assignee | ||
Comment 23•8 years ago
|
||
(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?
Comment 24•8 years ago
|
||
https://bugzilla.mozilla.org/show_bug.cgi?id=744830#c51 is one case.
![]() |
||
Comment 25•8 years ago
|
||
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
![]() |
||
Comment 26•8 years ago
|
||
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 27•8 years ago
|
||
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.
Assignee | ||
Comment 28•8 years ago
|
||
(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...
Assignee | ||
Comment 29•8 years ago
|
||
But I guess it's a sha-256 in hex form, which makes it indeed a 64 bytes string. The comment still applies, though.
Updated•8 years ago
|
Updated•8 years ago
|
Updated•8 years ago
|
![]() |
||
Comment 30•8 years ago
|
||
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 31•8 years ago
|
||
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.
Comment 32•8 years ago
|
||
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)
Assignee | ||
Comment 33•8 years ago
|
||
(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.
![]() |
||
Comment 34•8 years ago
|
||
> 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.
![]() |
Reporter | |
Comment 35•8 years ago
|
||
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.
![]() |
Reporter | |
Comment 36•8 years ago
|
||
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 37•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
Comment 38•8 years ago
|
||
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
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 39•8 years ago
|
||
(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).
![]() |
||
Comment 40•8 years ago
|
||
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. ;)
Assignee | ||
Comment 41•8 years ago
|
||
(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.
Assignee | ||
Comment 42•8 years ago
|
||
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.
Assignee | ||
Comment 43•8 years ago
|
||
Comment 44•8 years ago
|
||
bugherder |
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
![]() |
Reporter | |
Updated•8 years ago
|
Comment 45•8 years ago
|
||
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)
Comment 46•8 years ago
|
||
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.
Assignee | ||
Comment 47•8 years ago
|
||
(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.
Assignee | ||
Comment 48•8 years ago
|
||
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.
Assignee | ||
Comment 49•8 years ago
|
||
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 50•8 years ago
|
||
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
Updated•8 years ago
|
Assignee | ||
Comment 51•8 years ago
|
||
(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.
Assignee | ||
Comment 52•8 years ago
|
||
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).
Updated•8 years ago
|
Comment 54•7 years ago
|
||
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
Comment 55•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ffb38910dcae
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Comment 56•5 years ago
|
||
The leave-open keyword is there and there is no activity for 6 months. :glandium, maybe it's time to close this bug?
Comment 57•5 years ago
|
||
The leave-open keyword is there and there is no activity for 6 months.
:glandium, maybe it's time to close this bug?
Comment 58•4 years ago
|
||
The leave-open keyword is there and there is no activity for 6 months.
:glandium, maybe it's time to close this bug?
Comment 59•4 years ago
|
||
The leave-open keyword is there and there is no activity for 6 months.
:glandium, maybe it's time to close this bug?
Comment 60•3 years ago
|
||
The leave-open keyword is there and there is no activity for 6 months.
:glandium, maybe it's time to close this bug?
Updated•3 years ago
|
Comment 61•2 years ago
|
||
The leave-open keyword is there and there is no activity for 6 months.
:glandium, maybe it's time to close this bug?
Comment 62•2 years ago
|
||
The leave-open keyword is there and there is no activity for 6 months.
:glandium, maybe it's time to close this bug?
Comment 63•1 year ago
|
||
The leave-open keyword is there and there is no activity for 6 months.
:glandium, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.
Updated•1 year ago
|
Updated•1 year ago
|
Updated•8 months ago
|
Description
•