798914 - Consolidate MallocSizeOf typedefs

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

We've now got one in JS, one in XPCOM, one for our Chromium fork, and one for XPT. All of these depend on MFBT, so we should introduce a mozilla::MallocSizeOf there, and kill all the others.

Comment 1

[Nicholas Nethercote [inactive]]

I'm happy to do this.  My only question is:  where in mfbt/ should this live?  Types.h?  Util.h?  A new file?

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I'm pretty sure Waldo would argue for a new file :)

Comment 3

[Jeff Walden [:Waldo]]

Yup.  MemoryReporting.h maybe?  I don't know a whole lot about the memory reporting infrastructure, or exposed functionality/stuff, but I assume there's more to it than just a single typedef, and a new header for all of it would make sense.

Comment 4

[Nicholas Nethercote [inactive]]

It would only contain a single typedef.  I'm still happy to have a single file for it, though.

Comment 5

[Catalin Iacob]

I started doing this.

One issue is that xpt_arena.h is also meant to be included from C not just C++ so if the new MallocSizeOfFun is in namespace mozilla it can't be used by xpt_arena.h.

So do I (the options below are ordered by personal preference):
1. put MallocSizeOfFun in the mozilla namespace and don't reuse it in xpt?
2. put MallocSizeOfFun in the mozilla namespace only for C++ and in the global namespace for C? If so should it be named differently for C, something like mozMallocSizeOfFun?
3. don't put MallocSizeOfFun in the mozilla namespace and reuse it everywhere, including xpt?

Comment 6

[Catalin Iacob]

Actually, nsMallocSizeOfFun is defined in xpcom/base/nscore.h and this one also needs to be usable as a C header so I can't just include a C++ only MemoryReporting.h in it.

This means I would need to push the include of the new header down into all files that currently use nsMallocSizeOfFun.

Comment 7

[Catalin Iacob]

Attachment: part1

Comment 8

[Catalin Iacob]

Attachment: Part 2 - use MallocSizeOf in chromium

Comment 9

[Catalin Iacob]

Attachment: WIP, lacks includes Part 3 - replace nsMallocSizeOfFun with MallocSizeOf

Comment 10

[Catalin Iacob]

I've attached what I have so far so that I can get some validation for the general approach.

For the third patch I did the replacement automatically with sed but I still need to go through the files and include the new file everywhere to get everything to build. 

BTW, is it ok to add the same include to a .h and a .cpp file even though it's not really needed? It would be a lot easier to just add an #include "mozilla/MemoryReporting.h" to all files that I modified instead of adding it only to the files that really need it.

Comment 11

[Nicholas Nethercote [inactive]]

Thanks for working on this.  When you post a patch, please make sure you make a "review" or "feedback" request from somebody, otherwise you might not get a response.

mfbt/Util.h serves as a good example for this case.  I think you want something like this:

  #ifndef mozilla_MemoryReporting_h_
  #define mozilla_MemoryReporting_h_

  #ifdef __cplusplus

  namespace mozilla {
   
  // This is for functions that are like malloc_usable_size. Such functions are
  // used for measuring the size of data structures.
  typedef size_t(*MallocSizeOf)(const void *p);
   
  } // namespace mozilla

  #endif // __cpluscplus

  // This is an alternative to mozilla::MallocSizeOf for C code.
  typedef size_t(*MozMallocSizeOf)(const void *p);
   
  #endif // mozilla_MemoryReporting_h_

(mfbt/STYLE suggests the |Moz| prefix for C types.)

It makes me sad that the Mozilla namespace is |mozilla| and not |moz| :(

I think it's fine to have |#include "MemoryReporting.h"| in both Foo.h
and Foo.cpp, so long as |MallocSizeOf| occurs in both.  Compilers (GCC and
clang, at least) understand the #ifndef wrapper in MemoryReporting.h and can
avoid reading it more than once.

Finally, in C++ header files you'll need to use |mozilla::MallocSizeOf|.  But many .cpp files will have |using namespace mozilla;| in them, in which case you only need to write |MallocSizeOf|.  But some .cpp files lack the |using| statement.  So I'd adjust your script to omit the namespace qualification in .cpp files, and see how many compile failures you get, and then fix those ones by hand -- either by qualifying, or by adding the |using| statement.

Comment 12

[Nicholas Nethercote [inactive]]

Oh, and since MemoryReporting.h will be used in C code, it should only use C-style comments, not C++-style comments.  Thanks.

Comment 13

[Catalin Iacob]

Attachment: Part 1

Comment 14

[Catalin Iacob]

Attachment: Part 2

Comment 15

[Catalin Iacob]

Attachment: Part 3

Comment 16

[Catalin Iacob]

Attachment: Part 4

Comment 17

[Catalin Iacob]

Attachment: Part 5

Comment 18

[Catalin Iacob]

Attached current patches. Parts 4 and 5 are quite big, I generated them with a very hackish script and manually fixed 3 or 4 files. The script uses mozilla:: only if there is no using namespace mozilla in that file. It always includes MemoryReporting.h and it tries to put the include in a good place (near the other mozilla/.... includes, alphabetically, or as the first include if there isn't a mozilla/... include already).

The current script version is at https://github.com/cataliniacob/misc/blob/master/rename_moz_type.py

They were written against bb7c53fbb8cd from June 15 and rebased before uploading them, even in just 5 days I had to remove 4 more nsMallocSizeOfFun that trickled in.

This definitely needs a try run to check that it builds everywhere; I hope it does but you never know.

Comment 19

[Nicholas Nethercote [inactive]]

Comment on attachment 765600 [details] [diff] [review]
Part 1

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

::: mfbt/MemoryReporting.h
@@ +1,4 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

mfbt/STYLE doesn't mention modelines, so I'd follow https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style and change it to this:

  /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
  /* vim: set ts=8 sts=2 et sw=2 tw=80: */
  /* This Source Code Form is subject to the terms of the Mozilla Public
   * License, v. 2.0. If a copy of the MPL was not distributed with this
   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Comment 20

[Nicholas Nethercote [inactive]]

Comment on attachment 765601 [details] [diff] [review]
Part 2

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

Do you have commit access?  If not, I can try-server and land these patches for you.

Comment 21

[Nicholas Nethercote [inactive]]

Comment on attachment 765604 [details] [diff] [review]
Part 4

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

jsdhash.{h,cpp} were just removed in bug 884649.

https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style#.23include_ordering describes the SpiderMonkey #include ordering rules, which explains why I've asked you to add blank lines in various places.  (You might notice that not all the files currently follow these rules... it's because the rules are quite new and I'm in the process of fixing them.)

::: js/public/MemoryMetrics.h
@@ +9,5 @@
>  
>  // These declarations are not within jsapi.h because they are highly likely to
>  // change in the future. Depend on them at your own risk.
>  
> +#include "mozilla/MemoryReporting.h"

JS style says to put a blank line after this, before the <string.h>

::: js/public/Utility.h
@@ +9,5 @@
>  
>  #include "mozilla/Assertions.h"
>  #include "mozilla/Attributes.h"
>  #include "mozilla/Compiler.h"
> +#include "mozilla/MemoryReporting.h"

You don't need this.

::: js/src/ion/BaselineJIT.cpp
@@ +3,5 @@
>   * This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include "mozilla/MemoryReporting.h"

Blank line afterwards.

::: js/src/ion/BaselineJIT.h
@@ +8,5 @@
>  #define jsion_baseline_jit_h__
>  
>  #ifdef JS_ION
>  
> +#include "mozilla/MemoryReporting.h"

Blank line between this line and the jscntxt.h line.

::: js/src/ion/Ion.cpp
@@ +3,5 @@
>   * This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include "mozilla/MemoryReporting.h"

Blank line afterwards.

::: js/src/ion/Ion.h
@@ +8,5 @@
>  #define jsion_ion_h__
>  
>  #ifdef JS_ION
>  
> +#include "mozilla/MemoryReporting.h"

Again, blank line after.

::: js/src/ion/IonCompartment.h
@@ +8,5 @@
>  #define jsion_ion_compartment_h__
>  
>  #ifdef JS_ION
>  
> +#include "mozilla/MemoryReporting.h"

Blank line afterwards.

::: js/src/jsmath.h
@@ +6,5 @@
>  
>  #ifndef jsmath_h___
>  #define jsmath_h___
>  
> +#include "mozilla/MemoryReporting.h"

Blank line afterwards.

::: js/src/jsobj.h
@@ +14,5 @@
>   * ordered property names, called the map; and a dense vector of property
>   * values, called slots.  The map/slot pointer pair is GC'ed, while the map
>   * is reference counted and the slot vector is malloc'ed.
>   */
> +#include "mozilla/MemoryReporting.h"

Blank line afterwards.

::: js/src/vm/ArgumentsObject-inl.h
@@ +7,5 @@
>  #ifndef ArgumentsObject_inl_h___
>  #define ArgumentsObject_inl_h___
>  
>  #include "vm/ArgumentsObject.h"
> +#include "mozilla/MemoryReporting.h"

JS style is to put a blank line between vm/ArgumentsObject.h and what comes afterwards.

::: js/src/vm/ArgumentsObject.h
@@ +6,5 @@
>  
>  #ifndef ArgumentsObject_h___
>  #define ArgumentsObject_h___
>  
> +#include "mozilla/MemoryReporting.h"

Blank line afterwards.

::: js/src/vm/RegExpObject.cpp
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "vm/RegExpObject.h"
>  
> +#include "mozilla/MemoryReporting.h"

Blank line afterwards.

::: js/src/vm/RegExpStatics-inl.h
@@ +6,5 @@
>  
>  #ifndef RegExpStatics_inl_h__
>  #define RegExpStatics_inl_h__
>  
> +#include "mozilla/MemoryReporting.h"

Blank line afterwards.

::: js/xpconnect/src/XPCMaps.h
@@ +8,5 @@
>  
>  #ifndef xpcmaps_h___
>  #define xpcmaps_h___
>  
> +#include "mozilla/MemoryReporting.h"

Blank line afterwards.

Comment 22

[Nicholas Nethercote [inactive]]

Comment on attachment 765606 [details] [diff] [review]
Part 5

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

This touched a lot more files than I expected! :)  Thanks for doing it!

And thanks for going to the effort of putting the #include lines in order with other |#include "mozilla/*.h"| lines.

If you need me to land the patches, I'll do that once you've updated them for the review comments.  It's Friday here, so depending on timing I might not get to it until Monday next week.

::: xpcom/string/src/nsSubstring.cpp
@@ +8,5 @@
>  #define ENABLE_STRING_STATS
>  #endif
>  
>  #ifdef ENABLE_STRING_STATS
> +#include "mozilla/MemoryReporting.h"

This is in the wrong place -- please move it out of the ENABLE_STRING_STATS block.

(I may have missed other similar cases;  it would be worth doing a search for them if you can.)

Comment 23

[Jeff Walden [:Waldo]]

Comment on attachment 765600 [details] [diff] [review]
Part 1

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

Thanks for the patch!  Do you want to post a new one with the tweaks, then one of us can push it?

::: mfbt/MemoryReporting.h
@@ +1,4 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Yeah, we seem to be all over the map as far as mode lines go -- probably because not everyone cares about them, so they're only around inconsistently.  That'll do as well as anything til they all get cleaned up, I guess.

@@ +17,5 @@
> +/*
> + * This is for functions that are like malloc_usable_size.  Such functions are
> + * used for measuring the size of data structures.
> + */
> +typedef size_t(*MallocSizeOf)(const void *p);

There's truly no good way to format function pointer typedefs, but I think I can reasonably take a stand against mashing the return type in the signature up against the parentheses surrounding the type being defined.  :-)  It just makes the whole conjoined mess even more conjoined and more messy.  Also, * goes by the type per mfbt/STYLE.  So then you'd have:

typedef size_t (*MallocSizeOf)(const void* p);

And further down:

typedef size_t (*MozMallocSizeOf)(const void* p);

Comment 24

[Catalin Iacob]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #23)
> Yeah, we seem to be all over the map as far as mode lines go -- probably
> because not everyone cares about them, so they're only around
> inconsistently.  That'll do as well as anything til they all get cleaned up,
> I guess.

I updated this to Nick's suggestion from comment 19. If there is a documented standard it's better to follow it.

> typedef size_t (*MallocSizeOf)(const void* p);
> 
> And further down:
> 
> typedef size_t (*MozMallocSizeOf)(const void* p);

Updated, thanks. Should have read mfbt/STYLE more carefully, I only did a cursory read of it.

Comment 25

[Catalin Iacob]

(In reply to Nicholas Nethercote [:njn] from comment #21)
> https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style#.
> 23include_ordering describes the SpiderMonkey #include ordering rules, which
> explains why I've asked you to add blank lines in various places.

I updated all files in your list to add the blank lines and found another one where it was missing. In the mean time a new usage of JSMallocSizeOfFun popped up, I converted that as well.

> ::: js/public/Utility.h
> @@ +9,5 @@
> >  
> >  #include "mozilla/Assertions.h"
> >  #include "mozilla/Attributes.h"
> >  #include "mozilla/Compiler.h"
> > +#include "mozilla/MemoryReporting.h"
> 
> You don't need this.

True, removed.

Comment 26

[Catalin Iacob]

(In reply to Nicholas Nethercote [:njn] from comment #22)
> ::: xpcom/string/src/nsSubstring.cpp
> @@ +8,5 @@
> >  #define ENABLE_STRING_STATS
> >  #endif
> >  
> >  #ifdef ENABLE_STRING_STATS
> > +#include "mozilla/MemoryReporting.h"
> 
> This is in the wrong place -- please move it out of the ENABLE_STRING_STATS
> block.
> 
> (I may have missed other similar cases;  it would be worth doing a search
> for them if you can.)

Good catch. I searched for #ifdef in the whole patch and found another one like this in gfx/thebes/gfxFT2FontList.h in an #ifdef XP_WIN. Came with the automated adding of the header.

Comment 27

[Catalin Iacob]

Attachment: Part 1

Comment 28

[Catalin Iacob]

Attachment: Part 2

Comment 29

[Catalin Iacob]

Attachment: Part 3

Comment 30

[Catalin Iacob]

Attachment: Part 4

Comment 31

[Catalin Iacob]

Attachment: Part 5

Besides addressing the comments, this has a few more whitespace changes in gfx/thebes/gfxPlatformFontList.cpp and gfx/thebes/gfxPlatformFontList.h to align parameter names.

Comment 32

[Catalin Iacob]

Added new patches, these should address all comments and they're rebased on top of current mozilla-central.

I have neither try nor commit access. I'll ask on IRC for somebody to push these to try, so hopefully Monday morning somebody finds a green try run and lands these; the patches get stale quite quickly because others reintroduce uses of the replaced typedefs.

Comment 33

[Catalin Iacob]

Try run at https://tbpl.mozilla.org/?tree=Try&rev=235430b87bc7

Comment 34

[Nicholas Nethercote [inactive]]

That try run was green.  I'll do another try run (including debug builds, which are worth doing, in my experience) and then land.  Thanks.

Comment 35

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8f504ac6b83
https://hg.mozilla.org/integration/mozilla-inbound/rev/b77a391ef1fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/47512b0132a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d4480f6c25d
https://hg.mozilla.org/integration/mozilla-inbound/rev/727736b233b3

Thanks again, Catalin!

Comment 36

[[e-mail]]

Comment on attachment 766422 [details] [diff] [review]
Part 5

>diff --git a/xpcom/glue/nsCOMArray.cpp b/xpcom/glue/nsCOMArray.cpp
>--- a/xpcom/glue/nsCOMArray.cpp
>+++ b/xpcom/glue/nsCOMArray.cpp
>@@ -1,13 +1,14 @@
> /* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
>+#include "mozilla/MemoryReporting.h"
> #include "nsCOMArray.h"
What's correct style here? nsCOMArray.h has to include MemoryReporting.h to declare the function defined here.

Comment 37

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Correct style is that nsCOMArray.h is the first include in nsCOMArray.cpp.

Comment 38

[Nathan Froyd [:froydnj]]

(In reply to :Ms2ger from comment #37)
> Correct style is that nsCOMArray.h is the first include in nsCOMArray.cpp.

I think the point was more that nsCOMArray.h already includes MemoryReporting.h.

Comment 39

[Catalin Iacob]

Regarding having the include in the .cpp file and in the corresponding .h file, I think it's ok style wise if both files use the typedef, as it makes the files more self contained. The .cpp could be the only one using the typedef now or later. I find it weird that removing the include from the .h would require adding it to the .cpp. I realize in practice .cpp files end up relying on headers included only by their headers but I see that as a downside not something to strive for. I did ask about this in comment 10 and Nick agreed in comment 11.

The positions of the includes for Part 5 are decided by the automated script that generated them: together with the other mozilla/... includes if they exist or as the first header if not. I chose this as mfbt/STYLE says mfbt headers should be included first and I didn't realize that the corresponding header is special and should go on top.

I did manually fix js/ in Part 4 to have the corresponding .h first. Part 5 is considerably bigger though, and the include order in the files it touches is messier than in js/ to start with (for example mfbt includes aren't always first), so I didn't do it there. I can do that now if you think that's a good idea.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d8f504ac6b83
https://hg.mozilla.org/mozilla-central/rev/b77a391ef1fb
https://hg.mozilla.org/mozilla-central/rev/47512b0132a9
https://hg.mozilla.org/mozilla-central/rev/3d4480f6c25d
https://hg.mozilla.org/mozilla-central/rev/727736b233b3

Comment 41

[Nicholas Nethercote [inactive]]

> Correct style is that nsCOMArray.h is the first include in nsCOMArray.cpp.

True.  I forgot about that, sorry.

As for the #inclusion into both the .h and .cpp files, it doesn't worry me.  If you take the view that you shouldn't #include anything indirectly that you haven't #included directly, it's the right thing to do.

Comment 42

[Catalin Iacob]

Attachment: Followup, don't include MemoryReporting.h before a .cpp's own header

Finally had some time to address the concerns in comment 37 and comment 41.

This patch was generated by another hackish script, at https://github.com/cataliniacob/misc/blob/master/own_header_first.py

The script basically searches if the "own include comes first" was respected and the MemoryReporting.h include broke it.

In other words, if "MemoryReporting" is now the first include and the next include afterwards is the header's own include then it swaps them and adds newlines as appropriate to separate the include sections.

Comment 43

[Nicholas Nethercote [inactive]]

Comment on attachment 769492 [details] [diff] [review]
Followup, don't include MemoryReporting.h before a .cpp's own header

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

Thanks for doing this.  I like how you put a blank line after the module #include, to make it stand out.  I'll land this patch today.

Comment 44

[Nicholas Nethercote [inactive]]

Comment on attachment 766417 [details] [diff] [review]
Part 1

A minor thing:  when posting updates versions of r+'d patches, it's customary to r+ (yourself) the new versions.  That way it's clearer that the final versions have r+.  I'll do this for these patches now.

Comment 45

[Nicholas Nethercote [inactive]]

Part 6 (follow-up):
https://hg.mozilla.org/integration/mozilla-inbound/rev/23f0ca9de75f

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/23f0ca9de75f