Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8296774

Removed default MEMFLAGS value from CHeapBitMap



    • Enhancement
    • Resolution: Fixed
    • P4
    • 20
    • 20
    • hotspot
    • None
    • gc
    • b24


      Today it is easy to accidentally create CHeapBitMaps that uses the default mtInternal MEMFLAGS instead of a value that is suitable for the subsystem. I fixed the instances I could find with #10948 / JDK-8296231.

      For that PR I didn't want to change the constructors of the bitmap because #10941 / JDK-8296139 was being out for review. Now when that change has been pushed I'd like to change the constructors of the CHeapBitMap, so that we don't accidentally make these mistakes.

      When making it mandatory to pass MEMFLAGS, it becomes apparent that the current parameter order is a bit odd. If you look closely you see that all three parameters are optional. When I now want to make MEMFLAGS mandatory, I'd like to move it so that it always is the first parameter. This will simplify the constructors a bit, IMHO.

      This is what the constructors look like before the patch:
        CHeapBitMap() : CHeapBitMap(mtInternal) {}
        explicit CHeapBitMap(MEMFLAGS flags) : GrowableBitMap(0, false), _flags(flags) {}
        CHeapBitMap(idx_t size_in_bits, MEMFLAGS flags = mtInternal, bool clear = true);

      And I'd like to change it to:
        explicit CHeapBitMap(MEMFLAGS flags) : GrowableBitMap(0, false), _flags(flags) {}
        CHeapBitMap(MEMFLAGS flags, idx_t size_in_bits, bool clear = true);

      In effect, this makes `flags` mandatory and `size_in_bits` and `clear` optional.

      We could probably condense this even further into just one constructor:
        explicit CHeapBitMap(MEMFLAGS flags, size_t size_in_bits = 0, bool clear = true) : GrowableBitMap(size_in_bits, clear), _flags(flags) {}

      given that the value of `clear` doesn't matter when `size_in_bits` is 0. I didn't do that, but could be swayed to do that.


        Issue Links



              stefank Stefan Karlsson
              stefank Stefan Karlsson
              0 Vote for this issue
              3 Start watching this issue