I've been wanting to do that for a long time, but the trigger to do it today is that bug 900661 was mentioned on #developers. Bug 900661 is good example of how arbitary 32bitness in the interfaces of classes like nsTArray is causing code to be made harder to read (adding casts, see the patch there). Added casts make code harder to read in two ways: 1) visually and 2) making it harder to prove that no integer overflow can happen. nsTArray has (presumably) good reasons to use uint32's internally, to save memory. That just doesn't imply that its interface (e.g. the parameter to EnsureCapacity, the return type of Length, etc) should use uint32 too. Having the interface type be size_t while the internal storage type remains uint32 does mean added concerns of overflow, e.g. in EnsureCapacity(). Reviewers should be particularly careful here :) To make this easier to proofread wrt integer overflow, I used CheckedInt there. Note I only worried about EnsureCapacity, assuming that that would be enough. The rest of this patch is just mostly trivial adaptations.

Hm, already a negative review comment on my own patch: rather than including the nontrivial CheckedInt.h in nsTArray-inl.h, we should probably defer this to a non-inline function in the .cpp.

[19:26] <bjacob> froydnj: i found an issue in my patch. The value of NoIndex changes from uint32_t(-1) to size_t(-1). Code that calls IndexOf and casts the result to uint32_t is now broken. Going over callers now. Done that. Haven't yet addressed the above-mentioned issue of including CheckedInt.h in nsTArray-inl.h. I still think that it's probably a good idea to move that check to nsTArray.cpp, what do you think? Try push: https://tbpl.mozilla.org/?tree=Try&rev=8ea5ca35a41a

A good catch on linux build slaves, had forgotten a couple of IndexOf overflows. Also a nitpicky error on OSX. New try with that, and all tests now enabled, so hopefully if this causes a behavior difference this will be caught... https://tbpl.mozilla.org/?tree=Try&rev=8ea5ca35a41a

Sorry, the new try push is: https://tbpl.mozilla.org/?tree=Try&rev=c34277f5a0b8

Nevermind the CppUnit oranges with TestTArray failing: I forgot to update it from assuming that NoIndex == UINT32_MAX. Making it just use NoIndex. Not worth another try push, works locally.

Yup, this is coming out all green, should be considered ready for review!

I'm on PTO today, I will look at this carefully on Monday.

Comment on attachment 8415712 [details] [diff] [review] nstarray-sizes Review of attachment 8415712 [details] [diff] [review]: ----------------------------------------------------------------- I feel like a lot of the loops could make better use of the Contains(), RemoveElement(), or extended IndexOf functions we have in nsTArray. File a followup bug for that? I put some effort into verifying all the places we downcast to uint32_t with this patch are safe. How did you catch all of those? Did the compiler warn? (Do we know there aren't other places?) I think this patch is on the edge of requiring superreview, so flagging Benjamin. Even if it's not, Benjamin may have things to say as the owner of XPCOM. Feel free to cancel the review. ::: content/media/EncodedBufferCache.cpp @@ -29,5 @@ > if (mTempFileEnabled) { > // has created temporary file, write buffer in it > for (uint32_t i = 0; i < mEncodedBuffers.Length(); i++) { > - int64_t amount = PR_Write(mFD, mEncodedBuffers.ElementAt(i).Elements(), mEncodedBuffers.ElementAt(i).Length()); > - if (amount < mEncodedBuffers.ElementAt(i).Length()) { ...and this is why we try to fix signed-to-unsigned comparison warnings. ::: xpcom/glue/nsTArray-inl.h @@ +102,2 @@ > typename Alloc::ResultTypeProxy > nsTArray_base<Alloc, Copy>::EnsureCapacity(size_type capacity, size_type elemSize) { Since we now use CheckedInt for this function and since it doesn't get inlined anyway due to its size (unless PGO/LTO wants to inline it), would you file a followup bug for moving this into nsTArray.cpp? ::: xpcom/glue/nsTArray.h @@ +1692,5 @@ > // > // nsAutoArrayBase is a base class for AutoFallibleTArray and nsAutoTArray. > // You shouldn't use this class directly. > // > +template <class TArrayBase, size_t N> I don't know that this really matters, but...meh. ::: xpcom/tests/TestTArray.cpp @@ +599,5 @@ > + std::cout << __FILE__ << ":" << __LINE__ << " CHECK_EQ_INT(" \ > + << #actual << "=" << (actual) << ", " \ > + << #expected << "=" << (expected) << ") failed." \ > + << std::endl; \ > + return false; \ What is this change for? Is this so you don't have to bother with what-are-the-printf-specifiers-for-size_t-and-are-they-supported-on-my-platform?

(In reply to Nathan Froyd (:froydnj) from comment #8) > > ::: xpcom/glue/nsTArray-inl.h > @@ +102,2 @@ > > typename Alloc::ResultTypeProxy > > nsTArray_base<Alloc, Copy>::EnsureCapacity(size_type capacity, size_type elemSize) { > > Since we now use CheckedInt for this function and since it doesn't get > inlined anyway due to its size (unless PGO/LTO wants to inline it), would > you file a followup bug for moving this into nsTArray.cpp? Exactly what I was asking about above :) Sounds like I'll do it, yes. > > ::: xpcom/glue/nsTArray.h > @@ +1692,5 @@ > > // > > // nsAutoArrayBase is a base class for AutoFallibleTArray and nsAutoTArray. > > // You shouldn't use this class directly. > > // > > +template <class TArrayBase, size_t N> > > I don't know that this really matters, but...meh. I care about from the perspective of self-documenting code. uint32_t self-documents as "I only care about this having 32 bits or more, and being unsigned". For example, that can be nicely self-documenting for a bitflag parameter. But here, it wouldn't self-document usefully. size_t self-documents as "This will be a size or an index" which is exactly what we want here. > > ::: xpcom/tests/TestTArray.cpp > @@ +599,5 @@ > > + std::cout << __FILE__ << ":" << __LINE__ << " CHECK_EQ_INT(" \ > > + << #actual << "=" << (actual) << ", " \ > > + << #expected << "=" << (expected) << ") failed." \ > > + << std::endl; \ > > + return false; \ > > What is this change for? Is this so you don't have to bother with > what-are-the-printf-specifiers-for-size_t-and-are-they-supported-on-my- > platform? Exactly :-)

(In reply to Nathan Froyd (:froydnj) from comment #8) > Comment on attachment 8415712 [details] [diff] [review] > nstarray-sizes > > Review of attachment 8415712 [details] [diff] [review]: > ----------------------------------------------------------------- > > I feel like a lot of the loops could make better use of the Contains(), > RemoveElement(), or extended IndexOf functions we have in nsTArray. File a > followup bug for that? > > I put some effort into verifying all the places we downcast to uint32_t with > this patch are safe. How did you catch all of those? Did the compiler > warn? (Do we know there aren't other places?) Initially I forgot about that, and got startup crashes. Then I DXR'd for nsTArray::NoIndex and manually checked all the instances. DXR will still have missed any Windows-specific instance, which scares me a little. Given that missed cases typically give seriously wrong behavior, because this typically results in trying to do myarray[uint32_t(NoIndex)] i.e. myarray[2^32 - 1] i.e. addressing an array totally out-of-bounds, I hope that it would be caught on TBPL. At least in debug builds it would assert, and in 64bit release builds it would typically segfault. Less clear what would happen in 32bit windows release builds...

(In reply to Nathan Froyd (:froydnj) from comment #8) > Comment on attachment 8415712 [details] [diff] [review] > nstarray-sizes > > Review of attachment 8415712 [details] [diff] [review]: > ----------------------------------------------------------------- > > I feel like a lot of the loops could make better use of the Contains(), > RemoveElement(), or extended IndexOf functions we have in nsTArray. File a > followup bug for that? I don't feel that I know precisely enough what you mean; maybe you could file this bug :-)

Ah, I get it now. Filing a bug.

Filed Bug 1006148.

I have two basic questions: 1) Isn't it a bug that people are using size_t for nsTArray index types? We have the choice of either: a) papering over the mismatch by using casts, which is the approach from bug 900661 and is definitely ugly b) papering over the mismatch by making the external type a size_t and doing some cast and bounds checking internally, as in this bug c) actually fixing the mismatch by using size_t directly. d) actually fixing the mismatch by using uint32_t in client code. It seems preferable to me to do either c or d. You say "nsTArray has (presumably) good reasons to use uint32's internally" but I kinda doubt that we're doing it for memory reasons: I expect we're doing it to avoid inconsistencies when coding across platforms, or perhaps unlikely to keep word sizes correct when serializing things like IPDL structs across 32/64. If we can take this API change, we can probably just go all the way and use size_t. I'm just not sure it's better than consistently using uint32_t (or nsTArray::size_type if we want to be generic about it).

NI? back to froydnj/bjacob for thoughts.

I can't answer this without doing some measurements (how many nsTArrays do we have in memory? how many of them are non-empty? how much memory is saved by having them use uint32 internally?) Doing it now...

(In reply to Benoit Jacob [:bjacob] from comment #16) > I can't answer this without doing some measurements (how many nsTArrays do > we have in memory? how many of them are non-empty? how much memory is saved > by having them use uint32 internally?) > > Doing it now... This would really only matter for 64-bit platforms; I think we'd be doubling the size of nsTArray structures if we used size_t instead of 32-bit.

Start browser, default start page: There are 11961 array headers. The uint32-header optimization saves 8 bytes per array header, for a total of 95688 bytes. Load nytimes.com, scroll a bit: There are 28204 array headers. The uint32-header optimization saves 8 bytes per array header, for a total of 225632 bytes. Load 3 tabs: nytimes.com, cnn.com, youtube.com; scroll a bit on each: There are 66436 array headers. The uint32-header optimization saves 8 bytes per array header, for a total of 531488 bytes. Also load dev tools and select the inspector: There are 90353 array headers. The uint32-header optimization saves 8 bytes per array header, for a total of 722824 bytes. With 10 tabs: There are 160753 array headers. The uint32-header optimization saves 8 bytes per array header, for a total of 1286024 bytes.

Comment on attachment 8419484 [details] [diff] [review] Instrumentation to count array headers, and the memory overhead on 64bit if we made nstarrayheader use size_t internally Review of attachment 8419484 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/glue/nsTArray.h @@ +202,5 @@ > static void SizeTooBig(size_t) { > } > }; > > struct nsTArrayInfallibleAllocator : nsTArrayInfallibleAllocatorBase You didn't want to count infallible allocations?

Haha, sorry, pure omission :) will come back with hopefully correct numbers.

The updated result for the same 10 tabs case is: There are 195225 array headers. The uint32-header optimization saves 8 bytes per array header, for a total of 1561800 bytes. So roughly speaking, the corrected results are 20% higher than the earlier incorrect ones.

(In reply to Benoit Jacob [:bjacob] from comment #21) > So roughly speaking, the corrected results are 20% higher than the earlier > incorrect ones. Huh, I would have expected it to be higher, given that infallible is the default and you have to ask for fallibility. The optimization saves us ~1.5MB. I'm guessing that's ~0.5-1% of total memory. Benoit, could you obtain an about:memory dump from that browser, just to confirm that number? (I guess you don't have to take it from a browser with this patch, just from an empty profile.) If that amount is accurate, using uint32_t internally seems like a worthwhile improvement.

Yes, it seems that we should keep using uint32_t as the internal type. Given that, though, why wouldn't we also use uint32_t as the external type and fix the mismatch elsewhere?

(In reply to Benjamin Smedberg [:bsmedberg] from comment #23) > Yes, it seems that we should keep using uint32_t as the internal type. Given > that, though, why wouldn't we also use uint32_t as the external type and fix > the mismatch elsewhere? Could you elaborate on the difference in your options (a) and (d) in comment 14? I think comment 23 refers to option (d), but I'm not really sure what the difference is. You're not saying that if casts are required as in bug 900661 that you'd want to push the uint32_t interface outwards to callers-of-callers-of-...-callers-of-nsTArray code, are you?

Well, take the hunk in gfx/thebes/gfxGDIFontList.cpp from attachment 785472 [details] [diff] [review]. I think it's preferable to rewrite that code to loop using a uint32_t (or nsTArray::size_type) than to keep using size_t and take this patch. But I'm not sure how far down the rabbit hole that would take us, or how ugly the eventual interface would be. So I don't have a well-formed opinion.

I've sort of exceeded the amount of time that I could allow myself to work on non-gfx things :) so, if you want about:memory results fast, you're better of getting them yourself :) I too am surprised that the difference is only 20%, for the same reason as you. Maybe my patch is wrong; it's small though, so maybe inspection would reveal that. I feel strongly that the interface of container classes like nsTArray should use size_t, even if internally they use uint32_t as an optimization. Interface != internal storage. Propagating uint32 usage to the users is 1) making user code harder to read (including harder to proofread for integer overflow, patch on bug 900661), 2) making user code more warning-prone (also bug 900661, this is a major source of warnings on win32), 3) losing the nice self-documentation property of size_t, which says "this is an array index or length" without having to write any comment.

Another comment on the value of NOT exposing the internal indexing type (uint32) in the interface: We may want to switch internal indexing types more aggressively in the future, as internal optimizations of nsTArray and other containers. For example, the numbers obtained above, if correct, say that we have about 16k array headers per tab; obviously 99.99% of them must be of capacity less than 32k, so they could actually use uint16 internally; if we did that, we would save 4 bytes per header, so 4*16 == 64k per tab or per B2G application... which would be very appreciable on b2g. We would have to make this an internal automatic detail, because obviously we wouldn't want to have each user think hard about whether their array could possibly ever exceed 32k. If we dissociate the interface type from the internal indexing type, we give ourselves this freedom. If we hardcode that interface type == internal indexing type, then we can't do this class of automatic memory usage optimizations.

Comment on attachment 8415712 [details] [diff] [review] nstarray-sizes ok

^ For example, comment 28 would fit in a uint16_t.

Filed bug 1007846 for the uint16 plans.

Win64 build will be failure after this. c:\builds\moz2_slave\try-w64-0000000000000000000000\build\hal\windows/WindowsGamepad.cpp(748) : error C2780: 'const _Ty &std::min(const _Ty &,const _Ty &,_Pr)' : expects 3 arguments - 2 provided c:\builds\moz2_slave\try-w64-0000000000000000000000\build\hal\windows/WindowsGamepad.cpp(748) : error C2782: 'const _Ty &std::min(const _Ty &,const _Ty &)' : template parameter '_Ty' is ambiguous

fix win64 bustage

Comment on attachment 8419923 [details] [diff] [review] fix win64 bustage Review of attachment 8419923 [details] [diff] [review]: ----------------------------------------------------------------- ::: hal/windows/WindowsGamepad.cpp @@ +490,5 @@ > > void > WindowsGamepadService::ScanForDevices() > { > + for (size_t i = mGamepads.Length() - 1; i >= 0; i--) { The downwards loops in this file are now infinite, as size_t is unsigned. C++ is not making your life easy here :) Since this is to fix build bustage, I'd suggest you go for a minimal fix for now here, just casting around the std::min(), and then, if you have time, discuss with the maintainers of this file what approach they want to take to "do the right thing".

(In reply to Benoit Jacob [:bjacob] from comment #36) > Comment on attachment 8419923 [details] [diff] [review] > fix win64 bustage > > Review of attachment 8419923 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: hal/windows/WindowsGamepad.cpp > @@ +490,5 @@ > > > > void > > WindowsGamepadService::ScanForDevices() > > { > > + for (size_t i = mGamepads.Length() - 1; i >= 0; i--) { > > The downwards loops in this file are now infinite, as size_t is unsigned. > > C++ is not making your life easy here :) > > Since this is to fix build bustage, I'd suggest you go for a minimal fix for > now here, just casting around the std::min(), and then, if you have time, > discuss with the maintainers of this file what approach they want to take to > "do the right thing". I thought the usual fix in this case was to: for (size_t i = ...; i > 0; --i) { ...array[i-1]...; }

Ted, what are your thoughts on comment 36 and comment 37?

Also notice that this issue is unrelated to fixing the present build bustage: nsTArray indices were only changed from uint32_t to size_t, both are unsigned, there has been no signedness change here, there should be no need to change signedness of anything as a consequence.

Right, I was just using "int" in those loops (probably to avoid the warning). The loops definitely need to be counting down, since some of the logic inside can wind up removing the element at the current index. The suggestion in comment 37 is sort of horrible, but I don't have a better suggestion. Kinda wish we had iterators here.

If all you need here is the signed equivalent of size_t, then that's ptrdiff_t. What I'm not sure I understand is why that becomes needed now: 'int' was already different from the nsTArray index type before the present changes.

Totally agree on iterators :)

Right, but uint32_t and int are the same size, whereas size_t and int are not (on 64-bit platforms).

RyanVM noticed that we generate many more warnings than before on win64 compiles. I think these are representative: [1399646735.041, "build_output", {"line": "lloc,Copy>::ShrinkCapacity(nsTArray_base<Alloc,Copy>::size_type,size_t)' being compiled"}] [1399646735.042, "build_output", {"line": " with"}] [1399646735.042, "build_output", {"line": " ["}] [1399646735.042, "build_output", {"line": " Alloc=nsTArrayInfallibleAllocator,"}] [1399646735.043, "build_output", {"line": " Copy=nsTArray_CopyChooser<uint32_t>::Type"}] [1399646735.043, "build_output", {"line": " ]"}] [1399646735.043, "build_output", {"line": " c:\\mozbuild\\mozilla-central\\objdir-fx-64\\dist\\include\\nsTArray-inl.h(230) : while compiling class template member function 'void nsTArray_base<Alloc,Copy>::ShiftData(nsTArray_base<Alloc,Copy>::index_type,nsTArray_base<Alloc,Copy>::size_type,nsTArray_base<Alloc,Copy>::size_type,nsTArray_base<Alloc,Copy>::size_type,size_t)'"}] [1399646735.044, "build_output", {"line": " with"}] [1399646735.044, "build_output", {"line": " ["}] [1399646735.045, "build_output", {"line": " Alloc=nsTArrayInfallibleAllocator,"}] [1399646735.045, "build_output", {"line": " Copy=nsTArray_CopyChooser<uint32_t>::Type"}] [1399646735.045, "build_output", {"line": " ]"}] [1399646735.046, "build_output", {"line": " c:\\mozbuild\\mozilla-central\\objdir-fx-64\\dist\\include\\nsTArray.h(1303) : see reference to function template instantiation 'void nsTArray_base<Alloc,Copy>::ShiftData(nsTArray_base<Alloc,Copy>::index_type,nsTArray_base<Alloc,Copy>::size_type,nsTArray_base<Alloc,Copy>::size_type,nsTArray_base<Alloc,Copy>::size_type,size_t)' being compiled"}] [1399646735.047, "build_output", {"line": " with"}] [1399646735.047, "build_output", {"line": " ["}] [1399646735.048, "build_output", {"line": " Alloc=nsTArrayInfallibleAllocator,"}] [1399646735.048, "build_output", {"line": " Copy=nsTArray_CopyChooser<uint32_t>::Type"}] [1399646735.048, "build_output", {"line": " ]"}] [1399646735.05, "compiler_warning", {"column": null, "line": 427, "flag": "C4267", "message": "'+=' : conversion from 'size_t' to 'uint32_t', possible loss of data", "filename": "c:\\mozbuild\\mozilla-central\\objdir-fx-64\\dist\\include\\nsTArray.h"}] [1399646735.05, "build_output", {"line": "c:\\mozbuild\\mozilla-central\\objdir-fx-64\\dist\\include\\nsTArray.h(427) : warning C4267: '+=' : conversion from 'size_t' to 'uint32_t', possible loss of data"}] [1399646735.05, "build_output", {"line": " c:\\mozbuild\\mozilla-central\\objdir-fx-64\\dist\\include\\nsTArray.h(420) : while compiling class template member function 'void nsTArray_base<Alloc,Copy>::IncrementLength(size_t)'"}] [1399646735.051, "build_output", {"line": " with"}] [1399646735.051, "build_output", {"line": " ["}] [1399646735.051, "build_output", {"line": " Alloc=nsTArrayInfallibleAllocator,"}] [1399646735.052, "build_output", {"line": " Copy=nsTArray_CopyChooser<uint32_t>::Type"}] [1399646735.052, "build_output", {"line": " ]"}] [1399646735.052, "build_output", {"line": " c:\\mozbuild\\mozilla-central\\objdir-fx-64\\dist\\include\\nsTArray.h(1240) : see reference to function template instantiation 'void nsTArray_base<Alloc,Copy>::IncrementLength(size_t)' being compiled"}] [1399646735.053, "build_output", {"line": " with"}] [1399646735.053, "build_output", {"line": " ["}] [1399646735.055, "build_output", {"line": " Alloc=nsTArrayInfallibleAllocator,"}] [1399646735.056, "build_output", {"line": " Copy=nsTArray_CopyChooser<uint32_t>::Type"}] [1399646735.056, "build_output", {"line": " ]"}] These lines have to do with setting the sizes in the nsTArray's header. Since we're working with size_t indices (and lengths) now, but still have uint32_t members internally, we are truncating, and MSVC warns about this. Maybe it's time to disable that warning? Bits of the tree (webrtc, gfx/angle, jemalloc) already disable that warning. We don't turn on the equivalent warning (-Wconversion) for GCC, and whatever warning flags (-Wall etc.) we do use don't turn it on.

Interesting. I think that's actually a good warning, as truncation is an actual source of bugs (cf. the trouble I had above with NoIndex getting truncated). This seems worth fixing in nsTArray.h.

just bustage fix. Does I also add ptrdiff_t fix for compiler warning? Win64 builder doesn't use --enable-warnings-as-errors now.

Comment on attachment 8420734 [details] [diff] [review] just bustage fix just bustage fix just bustage fix. Does I also add ptrdiff_t fix for compiler warning? Win64 builder doesn't use --enable-warnings-as-errors now.

(In reply to Makoto Kato (:m_kato) from comment #46) > Created attachment 8420734 [details] [diff] [review] > just bustage fix > > just bustage fix. Does I also add ptrdiff_t fix for compiler warning? > Win64 builder doesn't use --enable-warnings-as-errors now. Changing these loop indices from 'int' to 'ptrdiff_t' is fine, and if it removes a warning about mixing integer types of different sizes, then it's a reasonable fix for it, yes.

(In reply to Benoit Jacob [:bjacob] from comment #48) > (In reply to Makoto Kato (:m_kato) from comment #46) > > Created attachment 8420734 [details] [diff] [review] > > just bustage fix > > > > just bustage fix. Does I also add ptrdiff_t fix for compiler warning? > > Win64 builder doesn't use --enable-warnings-as-errors now. > > Changing these loop indices from 'int' to 'ptrdiff_t' is fine, and if it > removes a warning about mixing integer types of different sizes, then it's a > reasonable fix for it, yes. OK, I will file a new bug to remove all warning from WindowsGamePad.cpp even if Win64. It also need more fix.

Comment on attachment 8420734 [details] [diff] [review] just bustage fix bustage fix is included in https://hg.mozilla.org/mozilla-central/rev/ead17eacbb43.

bjacob, could you post a PSA about this change to m.d.planning? We have tons of code (in the tree, and in-progress) that iterates/queries nsTArray via uint32_t-typed indices. I suspect Gecko developers would like to know that they should move to size_t for this, in new code. (And unless they happen to re-read the nsTArray.h header sometime soon, they won't necessarily notice.)

PSA was posted to dev.platform: https://groups.google.com/forum/#!topic/mozilla.dev.platform/ZAr3fHBpeLM

Ah, thanks! Sorry for the noise, then. (My local mail-search was insufficient; I just looked for "from:bjacob", but I now see that bjacob posts to dev-platform using an email address that does not include the string "bjacob". :))