1362977 - js::gc::CellSize is confusingly named

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

Heap cells have many different sizes, but I always assumed that CellSize was the minimum size.  In fact it's half the minimum size.  It is however the cell alignment.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1362977-cell-size

Rename the Cell in Cell{Shift,Size,Mask} to CellAlign.

Add a separate MinCellSize constant.

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8865476 [details] [diff] [review]
bug1362977-cell-size

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

::: js/public/HeapAPI.h
@@ +37,5 @@
>  const size_t ChunkMask = ChunkSize - 1;
>  
> +const size_t CellAlignShift = 3;
> +const size_t CellAlignSize = size_t(1) << CellAlignShift;
> +const size_t CellAlignMask = CellAlignSize - 1;

Oh wow. You're right, that was totally confusing.

But I don't see how CellAlignSize is a size at all. Why not call it CellAlignment (or CellAlign)?

I guess it's a size in that one bit in the mark bitmap represents CellAlignSize bytes. I'd kind of rather have a separate

    const size_t CellBytesPerBitmapBit = CellAlignment;

for that. I skimmed through all the uses, and it seems like the uses are about 50/50 between these two meanings, and it would make it more clear in most places. But this may still not be the best name for all uses. For example,

inline /* static */ size_t
ArenaCellSet::getCellIndex(const TenuredCell* cell)	
{
    MOZ_ASSERT((uintptr_t(cell) & ~ArenaMask) % CellAlignSize == 0);
    return (uintptr_t(cell) & ArenaMask) / CellSize;	
}

The first one (and any other expr % CellAlignSize) clearly should be CellAlignment. The second one would probably be

  return (uintptr_t(cell) & ArenaMask) / CellBytesPerBitmapBit;

but the connection between a "cell index" and a bitmap isn't immediately obvious. Hm... on the other hand, that's exactly what it is used for, so maybe it *is* a good name. (I suppose you could even s/getCellIndex/getCellMarkBitIndex/ if you wanted to be pedantic, but that may be scope creep here.)

::: js/src/gc/Heap.h
@@ +327,5 @@
> +const uintptr_t LargestTaggedNullCellPointer = (1 << CellAlignShift) - 1;
> +
> +/*
> + * The minimum cell size is twice the alignment because the mark bitmap conains
> + * one bit per CellAlignSize bytes and we need two mark bits per cell.

*contains

I think this is funky enough that it would be totally worth doing

  const size_t MarkBitsPerCell = 2;
  const size_t MinCellSize = CellAlignSize * MarkBitsPerCell;

to make the connection between the number of mark bits and the minimum cell size more clear.

@@ +337,5 @@
>      return (numerator + divisor - 1) / divisor;
>  }
>  
> +const size_t ArenaCellCount = ArenaSize / CellAlignSize;
> +static_assert(ArenaSize % CellAlignSize == 0, "Arena size must be a multiple of cell alignment");

ArenaCellCount doesn't make sense as a name. It is in no way a count of cells. And as far as I can tell, it is really only used for ArenaBitmapBits (it shows up other places, but afaict all of those places would be better off with s/ArenaCellCount/ArenaBitmapBits/).

If you're going to fix names, I think it would be better to eliminate ArenaCellCount.

Comment 3

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #2)
> But I don't see how CellAlignSize is a size at all. Why not call it
> CellAlignment (or CellAlign)?

Good point.  I'm going to call this CellAlignBytes.

> inline /* static */ size_t
> ArenaCellSet::getCellIndex(const TenuredCell* cell)	
> {
>     MOZ_ASSERT((uintptr_t(cell) & ~ArenaMask) % CellAlignSize == 0);
>     return (uintptr_t(cell) & ArenaMask) / CellSize;	
> }
> 
> The first one (and any other expr % CellAlignSize) clearly should be
> CellAlignment. The second one would probably be

This is checking that the division is exact so I think it makes sense to use the same constant.  (Except it's not because I messed up the assertion.  I'll fix that.)

> I think this is funky enough that it would be totally worth doing
> 
>   const size_t MarkBitsPerCell = 2;
>   const size_t MinCellSize = CellAlignSize * MarkBitsPerCell;

Good idea.

> ArenaCellCount doesn't make sense as a name. It is in no way a count of
> cells. And as far as I can tell, it is really only used for ArenaBitmapBits
> (it shows up other places, but afaict all of those places would be better
> off with s/ArenaCellCount/ArenaBitmapBits/).
> 
> If you're going to fix names, I think it would be better to eliminate
> ArenaCellCount.

Yes, this is really confusing too.

I'm going to change this to MaxArenaCellIndex and add ArenaCellIndexBytes (equal to CellAlignBytes) and change the places where we refer to cells by an index (the whole cell store buffer).  Really 'index' is wrong too but that will have to be another bug.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad92edd96284
Rename CellSize to CellAlignBytes and improve the naming of related constants r=sfink

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ad92edd96284