Move instance field to static

[DanielThomas]

SUMMARY

This adds an additional unnecessary reference to every BitmapContainer.

Automated Checks

• [ x ] I have run ./gradlew test and made sure that my PR does not break any unit test.
• [ x ] I have run ./gradlew checkstyleMain or the equivalent and corrected the formatting warnings reported.

[lemire]

Looks like a good catch.

[lemire]

@DanielThomas Could you also do:

RoaringBitmap/RoaringBitmap/src/main/java/org/roaringbitmap/buffer/MappeableBitmapContainer.java

Line 55 in dda04be

private final int MAXRUNS = (getArraySizeInBytes() - 2) / 4;

[DanielThomas]

@lemire I removed some duplication and switched in some size constants. Do you mind the cross-cutting package imports?

[DanielThomas]

Oops, let me fix this.

[richardstartin]

I guess nobody ever spotted this because bitmap containers contain an 8KiB array, so if you have enough of them for this slip up to show up noticeably, you would be distracted by hundreds of MiBs or even GiBs of long[] in the heap dump, but this is a great catch.

[DanielThomas]

YourKit's duplicate object inspection :)

[richardstartin]

Interestingly, this won't always amount to a footprint optimisation, because the stray field will usually fit into alignment shadow anyway (1.0.5)

System.err.println(ClassLayout.parseClass(BitmapContainer.class).toPrintable());

org.roaringbitmap.BitmapContainer object internals:
OFF  SZ     TYPE DESCRIPTION                   VALUE
  0   8          (object header: mark)         N/A
  8   4          (object header: class)        N/A
 12   4      int BitmapContainer.cardinality   N/A
 16   4      int BitmapContainer.MAXRUNS       N/A
 20   4   long[] BitmapContainer.bitmap        N/A
Instance size: 24 bytes
Space losses: 0 bytes internal + 0 bytes external = 0 bytes total

but will save 8 bytes per BitmapContainer on large heaps or with ZGC, whenever compressed oops doesn't apply:

org.roaringbitmap.BitmapContainer object internals:
OFF  SZ     TYPE DESCRIPTION                   VALUE
  0   8          (object header: mark)         N/A
  8   4          (object header: class)        N/A
 12   4      int BitmapContainer.cardinality   N/A
 16   4      int BitmapContainer.MAXRUNS       N/A
 20   4          (alignment/padding gap)       
 24   8   long[] BitmapContainer.bitmap        N/A
Instance size: 32 bytes
Space losses: 4 bytes internal + 0 bytes external = 4 bytes total

[lemire]

@DanielThomas Can you check the CI logs? I see...

  symbol:   variable MAX_CAPACITY
  location: class MappeableBitmapContainer
/home/runner/work/RoaringBitmap/RoaringBitmap/RoaringBitmap/src/main/java/org/roaringbitmap/buffer/BufferBitSetUtil.java:24: error: cannot find symbol
  static final private int BLOCK_LENGTH = MappeableBitmapContainer.MAX_CAPACITY / Long.SIZE; //
                                                                  ^

[lemire]

@DanielThomas This PR still does not build...

/home/runner/work/RoaringBitmap/RoaringBitmap/RoaringBitmap/src/main/java/org/roaringbitmap/longlong/Roaring64NavigableMap.java:1165: warning: [deprecation] MutableRoaringBitmapPrivate in org.roaringbitmap.buffer has been deprecated
        MutableRoaringBitmapPrivate.repairAfterLazy((MutableRoaringBitmap) lowBitmap);
        ^
/home/runner/work/RoaringBitmap/RoaringBitmap/RoaringBitmap/src/main/java/org/roaringbitmap/buffer/ImmutableRoaringArray.java:185: error: cannot find symbol
      bitmapArray.limit(MappeableBitmapContainer.MAX_CAPACITY / 64);
                                                ^
  symbol:   variable MAX_CAPACITY
  location: class MappeableBitmapContainer
/home/runner/work/RoaringBitmap/RoaringBitmap/RoaringBitmap/src/main/java/org/roaringbitmap/buffer/BufferBitSetUtil.java:24: error: cannot find symbol
  static final private int BLOCK_LENGTH = MappeableBitmapContainer.MAX_CAPACITY / Long.SIZE;

[DanielThomas]

Sorry @lemire, let me get that fixed.

[DanielThomas]

Based on your comments on the other PR about code duplication being a thing for the project, I've rolled back any other changes and restricted this to making the field in BitmapContainer static.

[lemire]

Based on your comments on the other PR about code duplication being a thing for the project, I've rolled back any other changes and restricted this to making the field in BitmapContainer static.

Note that it is not that we encourage code duplication; it is merely that, historically, we have accepted it in this library.

I do not discourage you from removing unnecessary code duplication. However, I prefer to be prudent with code refactoring that merely reduces code duplication because we can introduce new bugs by mistake.

[lemire]

We try to keep in sync BitmapContainer.java and MappeableBitmapContainer.java. Could you apply your changes to MappeableBitmapContainer.java.

See...

RoaringBitmap/RoaringBitmap/src/main/java/org/roaringbitmap/buffer/MappeableBitmapContainer.java

Line 55 in 5235aa6

private final int MAXRUNS = (getArraySizeInBytes() - 2) / 4;