Closed Bug 1414155 Opened 7 years ago Closed 7 years ago

Miscellaneous code changes related to size classes and constants/globals

Categories

(Core :: Memory Allocator, enhancement)

Product:
Core
Component:
Memory Allocator
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: glandium, Assigned: glandium)

Details

Attachments

(13 files)

Bug 1414155 - Move a few things around.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1414155 - Factor out size classes logic for tiny, quantum and sub-page classes.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1414155 - Move arena_chunk_map_t and arena_chunk_t around.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1414155 - Define pagesize_2pow in terms of pagesize, not the opposite.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1414155 - Consolidate "constant/globals".
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1414155 - Replace the cacheline-related macros with a constant.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1414155 - Remove SIZE_INV values for QUANTUM_2POW_MIN < 4.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1414155 - Replace size class related macros and constants.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1414155 - Replace chunk size related macros and constants.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1414155 - Rename page size related constants.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1414155 - Rename chunk related constants.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1414155 - Replace constants describing size class numbers.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1414155 - Define AddressRadixTree node size as a size rather than a power of 2.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details 
      No description provided.
Comment on attachment 8924836 [details]
Bug 1414155 - Replace size class related macros and constants.

https://reviewboard.mozilla.org/r/196088/#review201280


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: memory/build/mozjemalloc.cpp:4475
(Diff revision 1)
>  MozJemalloc::malloc_good_size(size_t aSize)
>  {
> -  if (aSize <= bin_maxclass) {
> +  if (aSize <= gMaxSubPageClass) {
>      // Small
>      return SizeClass(aSize);
> -  } else if (aSize <= arena_maxclass) {
> +  } else if (aSize <= gMaxLargeClass) {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
Comment on attachment 8924836 [details]
Bug 1414155 - Replace size class related macros and constants.

https://reviewboard.mozilla.org/r/196088/#review201282


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: memory/build/mozjemalloc.cpp:4475
(Diff revision 1)
>  MozJemalloc::malloc_good_size(size_t aSize)
>  {
> -  if (aSize <= bin_maxclass) {
> +  if (aSize <= gMaxSubPageClass) {
>      // Small
>      return SizeClass(aSize);
> -  } else if (aSize <= arena_maxclass) {
> +  } else if (aSize <= gMaxLargeClass) {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
Comment on attachment 8924829 [details]
Bug 1414155 - Move a few things around.

https://reviewboard.mozilla.org/r/196074/#review201298
Attachment #8924829 - Flags: review?(n.nethercote) → review+
Comment on attachment 8924830 [details]
Bug 1414155 - Factor out size classes logic for tiny, quantum and sub-page classes.

https://reviewboard.mozilla.org/r/196076/#review201302

::: memory/build/mozjemalloc.cpp:610
(Diff revision 1)
> +
> +  SizeClass& operator=(const SizeClass& aOther) = default;
> +
> +  bool operator==(const SizeClass& aOther) { return aOther.mSize == mSize; }
> +
> +  operator size_t() { return mSize; }

I'm not a fan of conversion operators like this, they tend to make code hard to read. I suggest replacing it with a Size() method. In a lot of places where you are using a SizeClass as a size_t you could instead use the aSize from which the SizeClass was created.
Attachment #8924830 - Flags: review?(n.nethercote) → review+
Comment on attachment 8924831 [details]
Bug 1414155 - Move arena_chunk_map_t and arena_chunk_t around.

https://reviewboard.mozilla.org/r/196078/#review201306
Attachment #8924831 - Flags: review?(n.nethercote) → review+
Comment on attachment 8924832 [details]
Bug 1414155 - Define pagesize_2pow in terms of pagesize, not the opposite.

https://reviewboard.mozilla.org/r/196080/#review201310

::: memory/build/Utils.h:34
(Diff revision 1)
>  
>    return (addr1 > addr2) - (addr1 < addr2);
>  }
>  
> +// User-defined literals to make constants more legible
> +constexpr unsigned long long int operator"" _KiB(unsigned long long int aNum)

Well... I just learned something!
Attachment #8924832 - Flags: review?(n.nethercote) → review+
(In reply to Nicholas Nethercote [:njn] from comment #19)
> I'm not a fan of conversion operators like this, they tend to make code hard
> to read. I suggest replacing it with a Size() method. In a lot of places
> where you are using a SizeClass as a size_t you could instead use the aSize
> from which the SizeClass was created.

Arguably, this class could be seen as a tagged integer...
Comment on attachment 8924833 [details]
Bug 1414155 - Consolidate "constant/globals".

https://reviewboard.mozilla.org/r/196082/#review201314

Yikes.

::: memory/build/mozjemalloc.cpp:464
(Diff revision 1)
> +#define GLOBAL_ASSERT MOZ_RELEASE_ASSERT
> +#endif
> +
> +DECLARE_GLOBAL(uint8_t, nsbins)
> +DECLARE_GLOBAL(uint8_t, pagesize_2pow)
> +DECLARE_GLOBAL(size_t, pagesize_mask)

Declare these in the same order they are defined below?
Attachment #8924833 - Flags: review?(n.nethercote) → review+
Comment on attachment 8924834 [details]
Bug 1414155 - Replace the cacheline-related macros with a constant.

https://reviewboard.mozilla.org/r/196084/#review201320

::: memory/build/mozjemalloc.cpp:385
(Diff revision 1)
>  #define CHUNK_2POW_DEFAULT 20
>  
>  // Maximum size of L1 cache line.  This is used to avoid cache line aliasing,
>  // so over-estimates are okay (up to a point), but under-estimates will
>  // negatively affect performance.
> -#define CACHELINE_2POW 6
> +static const size_t kCacheLine = 64;

kCacheLineSize?
Attachment #8924834 - Flags: review?(n.nethercote) → review+
Comment on attachment 8924835 [details]
Bug 1414155 - Remove SIZE_INV values for QUANTUM_2POW_MIN < 4.

https://reviewboard.mozilla.org/r/196086/#review201322
Attachment #8924835 - Flags: review?(n.nethercote) → review+
Comment on attachment 8924836 [details]
Bug 1414155 - Replace size class related macros and constants.

https://reviewboard.mozilla.org/r/196088/#review201368


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: memory/build/mozjemalloc.cpp:4477
(Diff revision 2)
>  MozJemalloc::malloc_good_size(size_t aSize)
>  {
> -  if (aSize <= bin_maxclass) {
> +  if (aSize <= gMaxSubPageClass) {
>      // Small
>      return SizeClass(aSize).Size();
> -  } else if (aSize <= arena_maxclass) {
> +  } else if (aSize <= gMaxLargeClass) {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
Comment on attachment 8924836 [details]
Bug 1414155 - Replace size class related macros and constants.

https://reviewboard.mozilla.org/r/196088/#review201618

Overall I like this patch, but I have a number of comments.

::: memory/build/mozjemalloc.cpp:394
(Diff revision 3)
>  #else
> -#define TINY_MIN_2POW (sizeof(void*) == 8 ? 3 : 2)
> +static const size_t kMinTinyClass = sizeof(void*);
>  #endif
>  
> -// Maximum size class that is a multiple of the quantum, but not (necessarily)
> -// a power of 2.  Above this size, allocations are rounded up to the nearest
> +// Maximum tiny size class.
> +static const size_t kMaxTinyClass = 8;

This reminds me that the comment at the top of mozjemalloc.cpp still claims that 2 and 4 byte allocations are possible. Fix that?

::: memory/build/mozjemalloc.cpp:397
(Diff revision 3)
>  
> -// Maximum size class that is a multiple of the quantum, but not (necessarily)
> -// a power of 2.  Above this size, allocations are rounded up to the nearest
> -// power of 2.
> -#define SMALL_MAX_2POW_DEFAULT 9
> -#define SMALL_MAX_DEFAULT (1U << SMALL_MAX_2POW_DEFAULT)
> +// Maximum tiny size class.
> +static const size_t kMaxTinyClass = 8;
> +
> +// Amount (quantum) separating quantum-spaced size classes.
> +static const size_t kQuantum = 16;

Huh. SMALL_MAX_DEFAULT was 512, which doesn't match the comment at the top of mozjemalloc.cpp!

Have you effectively removed the "small" terminology? If so, please remove it from the comment at the top. If not, it might be worth checking to see if it's being used consistently...

::: memory/build/mozjemalloc.cpp:400
(Diff revision 3)
> -// power of 2.
> -#define SMALL_MAX_2POW_DEFAULT 9
> -#define SMALL_MAX_DEFAULT (1U << SMALL_MAX_2POW_DEFAULT)
> -
> -// Various quantum-related settings.
> -#define QUANTUM_DEFAULT (size_t(1) << QUANTUM_2POW_MIN)
> +
> +// Amount (quantum) separating quantum-spaced size classes.
> +static const size_t kQuantum = 16;
> +
> +// Smallest and largest quantum-spaced size classes.
> +static const size_t kMinQuantumClass = kMaxTinyClass * 2;

Could this just be 16? Is there some genuine significance to the `kMaxTinyClass * 2` value?

::: memory/build/mozjemalloc.cpp:407
(Diff revision 3)
> -// Various bin-related settings.
> +              "kMaxQuantumClass is not a multiple of kQuantum");
> -static const size_t small_min = (QUANTUM_DEFAULT >> 1) + 1;
> -static const size_t small_max = size_t(SMALL_MAX_DEFAULT);
>  
>  // Number of (2^n)-spaced tiny bins.
> -static const unsigned ntbins = unsigned(QUANTUM_2POW_MIN - TINY_MIN_2POW);
> +static const unsigned ntbins =

In practice this is either 0 or 1, right? Might be worth saying that in the comment.

::: memory/build/mozjemalloc.cpp:463
(Diff revision 3)
>  DECLARE_GLOBAL(uint8_t, nsbins)
> -DECLARE_GLOBAL(size_t, bin_maxclass)
> +DECLARE_GLOBAL(uint8_t, pagesize_2pow)
> +DECLARE_GLOBAL(size_t, pagesize_mask)
>  DECLARE_GLOBAL(size_t, chunk_npages)
>  DECLARE_GLOBAL(size_t, arena_chunk_header_npages)
> -DECLARE_GLOBAL(size_t, arena_maxclass)
> +DECLARE_GLOBAL(size_t, gMaxLargeClass)

Should these have kFoo form or gFoo form? They're globals in one configuration and constants in another. But even when they are globals, they never change, right? So I'd lean towards kFoo.

::: memory/build/mozjemalloc.cpp:463
(Diff revision 3)
>  DECLARE_GLOBAL(uint8_t, nsbins)
> -DECLARE_GLOBAL(size_t, bin_maxclass)
> +DECLARE_GLOBAL(uint8_t, pagesize_2pow)
> +DECLARE_GLOBAL(size_t, pagesize_mask)
>  DECLARE_GLOBAL(size_t, chunk_npages)
>  DECLARE_GLOBAL(size_t, arena_chunk_header_npages)
> -DECLARE_GLOBAL(size_t, arena_maxclass)
> +DECLARE_GLOBAL(size_t, gMaxLargeClass)

You removed arena_maxclass, but you kept bin_maxclass (albeit renamed as gMaxBinClass). But gMaxBinClass is just another name for gMaxSubPageClass, so maybe just remove gMaxBinClass?

::: memory/build/mozjemalloc.cpp:2249
(Diff revision 3)
>    // We can only use TLS if this is a PIC library, since for the static
>    // library version, libc's malloc is used by TLS allocation, which
>    // introduces a bootstrapping issue.
>  
> -  // Only use a thread local arena for small sizes.
> -  if (size <= small_max) {
> +  // Only use a thread local arena for quantum and tiny sizes.
> +  if (size <= kMaxQuantumClass) {

Here's another case where "small" was excluding sub-page, disagreeing with the comment at the top. Good that you've removed it!

::: memory/build/mozjemalloc.cpp:2330
(Diff revision 3)
>  // becomes
>  //
> -//   (X * size_invs[(D >> QUANTUM_2POW_MIN) - 3]) >> SIZE_INV_SHIFT
> +//   (X * size_invs[(D / kQuantum) - 3]) >> SIZE_INV_SHIFT
>  
>  #define SIZE_INV_SHIFT 21
> -#define SIZE_INV(s) (((1U << SIZE_INV_SHIFT) / (s << QUANTUM_2POW_MIN)) + 1)
> +#define SIZE_INV(s) (((1U << SIZE_INV_SHIFT) / (s * kQuantum)) + 1)

I'm undecided about whether `(s << LOG2(kQuantum))` would be clearer...

::: memory/build/mozjemalloc.cpp:2380
(Diff revision 3)
>      } else {
>        // The run size is too large for us to use the lookup
>        // table.  Use real division.
>        regind = diff / size;
>      }
> -  } else if (size <=
> +  } else if (size <= ((sizeof(size_invs) / sizeof(unsigned)) * kQuantum) + 2) {

ditto

::: memory/build/mozjemalloc.cpp:3118
(Diff revision 3)
>  {
>    MOZ_DIAGNOSTIC_ASSERT(mMagic == ARENA_MAGIC);
>    MOZ_ASSERT(aSize != 0);
> -  MOZ_ASSERT(QUANTUM_CEILING(aSize) <= arena_maxclass);
> +  MOZ_ASSERT(QUANTUM_CEILING(aSize) <= gMaxLargeClass);
>  
> -  return (aSize <= bin_maxclass) ? MallocSmall(aSize, aZero)
> +  return (aSize <= gMaxBinClass) ? MallocSmall(aSize, aZero)

Hmm, this is a case where "small" does include sub-page allocations. Sigh.

::: memory/build/mozjemalloc.cpp:4515
(Diff revision 3)
>  
>    // Gather runtime settings.
>    aStats->opt_junk = opt_junk;
>    aStats->opt_zero = opt_zero;
> -  aStats->quantum = quantum;
> -  aStats->small_max = small_max;
> +  aStats->quantum = kQuantum;
> +  aStats->small_max = kMaxQuantumClass;

This "small" excludes sub-page.
Attachment #8924836 - Flags: review?(n.nethercote) → review+
Comment on attachment 8924837 [details]
Bug 1414155 - Replace chunk size related macros and constants.

https://reviewboard.mozilla.org/r/196090/#review201624

::: memory/build/mozjemalloc.cpp:412
(Diff revision 1)
>  static const unsigned nqbins = unsigned(kMaxQuantumClass / kQuantum);
>  
> -#define CHUNKSIZE_DEFAULT ((size_t)1 << CHUNK_2POW_DEFAULT)
> -static const size_t chunksize = CHUNKSIZE_DEFAULT;
> -static const size_t chunksize_mask = CHUNKSIZE_DEFAULT - 1;
> +// Size and alignment of memory chunks that are allocated by the OS's virtual
> +// memory system.
> +static const size_t kChunkSize = 1_MiB;
> +static const size_t kChunkSizeMask = kChunkSize - 1;

In the previous patch you removed the quantum mask and replaced it with `kQuantum - 1`, but here you've kept the chunk mask. Maybe put the quantum mask back in for consistency?
Attachment #8924837 - Flags: review?(n.nethercote) → review+
Comment on attachment 8924841 [details]
Bug 1414155 - Define AddressRadixTree node size as a size rather than a power of 2.

https://reviewboard.mozilla.org/r/196098/#review201626
Attachment #8924841 - Flags: review?(n.nethercote) → review+
(In reply to Nicholas Nethercote [:njn] from comment #51)
> > +static const size_t kMaxTinyClass = 8;
> 
> This reminds me that the comment at the top of mozjemalloc.cpp still claims
> that 2 and 4 byte allocations are possible. Fix that?

Actually, 4 byte allocations are possible. Not 2 byte, though.
 
> ::: memory/build/mozjemalloc.cpp:397
> (Diff revision 3)
> >  
> > -// Maximum size class that is a multiple of the quantum, but not (necessarily)
> > -// a power of 2.  Above this size, allocations are rounded up to the nearest
> > -// power of 2.
> > -#define SMALL_MAX_2POW_DEFAULT 9
> > -#define SMALL_MAX_DEFAULT (1U << SMALL_MAX_2POW_DEFAULT)
> > +// Maximum tiny size class.
> > +static const size_t kMaxTinyClass = 8;
> > +
> > +// Amount (quantum) separating quantum-spaced size classes.
> > +static const size_t kQuantum = 16;
> 
> Huh. SMALL_MAX_DEFAULT was 512, which doesn't match the comment at the top
> of mozjemalloc.cpp!
> 
> Have you effectively removed the "small" terminology? If so, please remove
> it from the comment at the top. If not, it might be worth checking to see if
> it's being used consistently...

The small terminology stays, and with this patch should be consistently matching the comment at the top, including sub-page.

> ::: memory/build/mozjemalloc.cpp:400
> (Diff revision 3)
> > -// power of 2.
> > -#define SMALL_MAX_2POW_DEFAULT 9
> > -#define SMALL_MAX_DEFAULT (1U << SMALL_MAX_2POW_DEFAULT)
> > -
> > -// Various quantum-related settings.
> > -#define QUANTUM_DEFAULT (size_t(1) << QUANTUM_2POW_MIN)
> > +
> > +// Amount (quantum) separating quantum-spaced size classes.
> > +static const size_t kQuantum = 16;
> > +
> > +// Smallest and largest quantum-spaced size classes.
> > +static const size_t kMinQuantumClass = kMaxTinyClass * 2;
> 
> Could this just be 16? Is there some genuine significance to the
> `kMaxTinyClass * 2` value?

There actually is. Tiny classes are powers of two. The smallest quantum size could, technically, be labelled as a tiny.

> ::: memory/build/mozjemalloc.cpp:407
> (Diff revision 3)
> > -// Various bin-related settings.
> > +              "kMaxQuantumClass is not a multiple of kQuantum");
> > -static const size_t small_min = (QUANTUM_DEFAULT >> 1) + 1;
> > -static const size_t small_max = size_t(SMALL_MAX_DEFAULT);
> >  
> >  // Number of (2^n)-spaced tiny bins.
> > -static const unsigned ntbins = unsigned(QUANTUM_2POW_MIN - TINY_MIN_2POW);
> > +static const unsigned ntbins =
> 
> In practice this is either 0 or 1, right? Might be worth saying that in the
> comment.

or 2.

> ::: memory/build/mozjemalloc.cpp:463
> (Diff revision 3)
> >  DECLARE_GLOBAL(uint8_t, nsbins)
> > -DECLARE_GLOBAL(size_t, bin_maxclass)
> > +DECLARE_GLOBAL(uint8_t, pagesize_2pow)
> > +DECLARE_GLOBAL(size_t, pagesize_mask)
> >  DECLARE_GLOBAL(size_t, chunk_npages)
> >  DECLARE_GLOBAL(size_t, arena_chunk_header_npages)
> > -DECLARE_GLOBAL(size_t, arena_maxclass)
> > +DECLARE_GLOBAL(size_t, gMaxLargeClass)
> 
> Should these have kFoo form or gFoo form? They're globals in one
> configuration and constants in another. But even when they are globals, they
> never change, right? So I'd lean towards kFoo.

I leant towards gFoo because it makes it immediately evident in the code that we're dealing with not-actually-constant constants, meaning we should avoid division by those values, etc. (which is why pagesize_2pow is the only 2pow that stays, because >> pagesize_2pow is significantly faster than / pagesize ; with constants, the compiler turns the division into bit shifts on its own, but with non-constants, it can't).
 
> ::: memory/build/mozjemalloc.cpp:463
> (Diff revision 3)
> >  DECLARE_GLOBAL(uint8_t, nsbins)
> > -DECLARE_GLOBAL(size_t, bin_maxclass)
> > +DECLARE_GLOBAL(uint8_t, pagesize_2pow)
> > +DECLARE_GLOBAL(size_t, pagesize_mask)
> >  DECLARE_GLOBAL(size_t, chunk_npages)
> >  DECLARE_GLOBAL(size_t, arena_chunk_header_npages)
> > -DECLARE_GLOBAL(size_t, arena_maxclass)
> > +DECLARE_GLOBAL(size_t, gMaxLargeClass)
> 
> You removed arena_maxclass, but you kept bin_maxclass (albeit renamed as
> gMaxBinClass). But gMaxBinClass is just another name for gMaxSubPageClass,
> so maybe just remove gMaxBinClass?

I intentionally left it because things like bug 1399695 could make sub-page allocations not go into bins, and it's more convenient to be able to tweak what gMaxBinClass points to than to change all the relevant gMaxSubPageClass references.

> ::: memory/build/mozjemalloc.cpp:2330
> (Diff revision 3)
> >  // becomes
> >  //
> > -//   (X * size_invs[(D >> QUANTUM_2POW_MIN) - 3]) >> SIZE_INV_SHIFT
> > +//   (X * size_invs[(D / kQuantum) - 3]) >> SIZE_INV_SHIFT
> >  
> >  #define SIZE_INV_SHIFT 21
> > -#define SIZE_INV(s) (((1U << SIZE_INV_SHIFT) / (s << QUANTUM_2POW_MIN)) + 1)
> > +#define SIZE_INV(s) (((1U << SIZE_INV_SHIFT) / (s * kQuantum)) + 1)
> 
> I'm undecided about whether `(s << LOG2(kQuantum))` would be clearer...

I tend to find multiplication and division more obvious than bit shifts.
 
> ::: memory/build/mozjemalloc.cpp:4515
> (Diff revision 3)
> >  
> >    // Gather runtime settings.
> >    aStats->opt_junk = opt_junk;
> >    aStats->opt_zero = opt_zero;
> > -  aStats->quantum = quantum;
> > -  aStats->small_max = small_max;
> > +  aStats->quantum = kQuantum;
> > +  aStats->small_max = kMaxQuantumClass;
> 
> This "small" excludes sub-page.

I didn't want to change jemalloc_stats_t just yet, but I guess nothing is using this field, so it doesn't matter if it's renamed.

(In reply to Nicholas Nethercote [:njn] from comment #52)
> Comment on attachment 8924837 [details]
> Bug 1414155 - Replace chunk size related macros and constants.
> 
> https://reviewboard.mozilla.org/r/196090/#review201624
> 
> ::: memory/build/mozjemalloc.cpp:412
> (Diff revision 1)
> >  static const unsigned nqbins = unsigned(kMaxQuantumClass / kQuantum);
> >  
> > -#define CHUNKSIZE_DEFAULT ((size_t)1 << CHUNK_2POW_DEFAULT)
> > -static const size_t chunksize = CHUNKSIZE_DEFAULT;
> > -static const size_t chunksize_mask = CHUNKSIZE_DEFAULT - 1;
> > +// Size and alignment of memory chunks that are allocated by the OS's virtual
> > +// memory system.
> > +static const size_t kChunkSize = 1_MiB;
> > +static const size_t kChunkSizeMask = kChunkSize - 1;
> 
> In the previous patch you removed the quantum mask and replaced it with
> `kQuantum - 1`, but here you've kept the chunk mask. Maybe put the quantum
> mask back in for consistency?

kChunkSizeMask is used all over the place. kQuantumMask would only have one use, in QUANTUM_CEILING.
Flags: needinfo?(n.nethercote)
Comment 54's explanations all seem reasonable...

> kChunkSizeMask is used all over the place. kQuantumMask would only have one use, in QUANTUM_CEILING.

...though if I picked one point to defend it would be this one. It is used twice, albeit on one line :)
Flags: needinfo?(n.nethercote)
Comment on attachment 8924838 [details]
Bug 1414155 - Rename page size related constants.

https://reviewboard.mozilla.org/r/196092/#review201324

::: memory/build/mozjemalloc.cpp:420
(Diff revision 1)
>  // VM page size. It must divide the runtime CPU page size or the code
>  // will abort.
>  // Platform specific page size conditions copied from js/public/HeapAPI.h
>  #if (defined(SOLARIS) || defined(__FreeBSD__)) &&                              \
>    (defined(__sparc) || defined(__sparcv9) || defined(__ia64))
> -static const size_t pagesize = 8_KiB;
> +static const size_t gPageSize = 8_KiB;

Shouldn't these be kFoo instead of gFoo?
Attachment #8924838 - Flags: review?(n.nethercote) → review+
Comment on attachment 8924839 [details]
Bug 1414155 - Rename chunk related constants.

https://reviewboard.mozilla.org/r/196094/#review201326
Attachment #8924839 - Flags: review?(n.nethercote) → review+
Comment on attachment 8924840 [details]
Bug 1414155 - Replace constants describing size class numbers.

https://reviewboard.mozilla.org/r/196096/#review201654

::: memory/build/mozjemalloc.cpp:454
(Diff revision 2)
>  #define DEFINE_GLOBAL(type)
>  #define GLOBAL_LOG2 FloorLog2
>  #define GLOBAL_ASSERT MOZ_RELEASE_ASSERT
>  #endif
>  
> -DECLARE_GLOBAL(uint8_t, nsbins)
> +DECLARE_GLOBAL(uint8_t, gNumSubPageClasses)

Should this be size_t?

::: memory/build/mozjemalloc.cpp:471
(Diff revision 2)
>  // Largest sub-page size class.
>  DEFINE_GLOBAL(size_t) gMaxSubPageClass = gPageSize / 2;
>  
> -// Number of (2^n)-spaced sub-page bins.
> +// Number of (2^n)-spaced sub-page classes.
>  DEFINE_GLOBAL(uint8_t)
> -nsbins = LOG2(gMaxSubPageClass) - LOG2(kMaxQuantumClass);
> +gNumSubPageClasses = GLOBAL_LOG2(gMaxSubPageClass) - LOG2(kMaxQuantumClass);

What's the effect of changing to GLOBAL_LOG2? Would
Attachment #8924840 - Flags: review?(n.nethercote) → review+
BTW, I'm feeling very happy with the decision to give up on jemalloc4 and commit to mozjemalloc, for the following reasons.

- Readability and safety: the conversion from old-school C to Mozilla-style C++.

- Simplicity: the removal of all the configuration knobs we don't need, and the simplification of all the replace-malloc stuff.

- Control: the ability to add custom stuff like moz_arena_*() and jemalloc_ptr_info().

- Performance: potential future reductions in book-keeping data size and fragmentation by choosing better run sizes.

- Stability: not having to worry about updates causing performance or stability concerns.
> Shouldn't these be kFoo instead of gFoo?

Out-of-date comment; please ignore.
(In reply to Nicholas Nethercote [:njn] from comment #58)
> Comment on attachment 8924840 [details]
> Bug 1414155 - Replace constants describing size class numbers.
> 
> https://reviewboard.mozilla.org/r/196096/#review201654
> 
> ::: memory/build/mozjemalloc.cpp:454
> (Diff revision 2)
> >  #define DEFINE_GLOBAL(type)
> >  #define GLOBAL_LOG2 FloorLog2
> >  #define GLOBAL_ASSERT MOZ_RELEASE_ASSERT
> >  #endif
> >  
> > -DECLARE_GLOBAL(uint8_t, nsbins)
> > +DECLARE_GLOBAL(uint8_t, gNumSubPageClasses)
> 
> Should this be size_t?

There can't be that many classes. The most there can be, in practice is 6, assuming a page size of 64K.

> ::: memory/build/mozjemalloc.cpp:471
> (Diff revision 2)
> >  // Largest sub-page size class.
> >  DEFINE_GLOBAL(size_t) gMaxSubPageClass = gPageSize / 2;
> >  
> > -// Number of (2^n)-spaced sub-page bins.
> > +// Number of (2^n)-spaced sub-page classes.
> >  DEFINE_GLOBAL(uint8_t)
> > -nsbins = LOG2(gMaxSubPageClass) - LOG2(kMaxQuantumClass);
> > +gNumSubPageClasses = GLOBAL_LOG2(gMaxSubPageClass) - LOG2(kMaxQuantumClass);
> 
> What's the effect of changing to GLOBAL_LOG2? Would

Err, this was done on the wrong patch. It doesn't build with LOG2 because it's not a constant.
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/4a93fcad5d8e
Move a few things around. r=njn
https://hg.mozilla.org/integration/autoland/rev/0b3250499ed0
Factor out size classes logic for tiny, quantum and sub-page classes. r=njn
https://hg.mozilla.org/integration/autoland/rev/9ee0115104a4
Move arena_chunk_map_t and arena_chunk_t around. r=njn
https://hg.mozilla.org/integration/autoland/rev/fb8fa3f4449c
Define pagesize_2pow in terms of pagesize, not the opposite. r=njn
https://hg.mozilla.org/integration/autoland/rev/5bd88f059878
Consolidate "constant/globals". r=njn
https://hg.mozilla.org/integration/autoland/rev/f6799dfca63c
Replace the cacheline-related macros with a constant. r=njn
https://hg.mozilla.org/integration/autoland/rev/5617b3050501
Remove SIZE_INV values for QUANTUM_2POW_MIN < 4. r=njn
https://hg.mozilla.org/integration/autoland/rev/9429ad8b651f
Replace size class related macros and constants. r=njn
https://hg.mozilla.org/integration/autoland/rev/578f15a2e0f6
Replace chunk size related macros and constants. r=njn
https://hg.mozilla.org/integration/autoland/rev/9d00f5278383
Rename page size related constants. r=njn
https://hg.mozilla.org/integration/autoland/rev/a826adc9b1c7
Rename chunk related constants. r=njn
https://hg.mozilla.org/integration/autoland/rev/c84122339c7f
Replace constants describing size class numbers. r=njn
https://hg.mozilla.org/integration/autoland/rev/849017ffe297
Define AddressRadixTree node size as a size rather than a power of 2. r=njn
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: