RFR: 8251158: Implementation of JEP 387: Elastic Metaspace

Leo Korinth leo.korinth at oracle.com
Thu Aug 20 10:24:24 UTC 2020


Hi!

Thanks for doing this, it does indeed look like a great improvement to
me, also many thanks for the /great/ documentation of your change.

Until I start my review on each part, I have a few general comments.

1) I believe structs should be named the same way as classes
    (UpperCamelCase) and not end with a "_t" (a naming convention that
    at least to me signals a typedef)
    
2) struct fields (and public class fields should to my knowledge be
    prefixed with underscore), there should be no naming difference
    between public and private fields.
    
3) I can see that you make most struct classes without
    constructors. Maybe there is some issue with
    trivial/standard-layout of classes, but otherwise I think
    constructors are really helpful. They both force you to initialize
    data and also gives your IDE help to warn you about uninitialized
    fields in the constructor.
    
4) As our build system (unfortunately) discard the path and thus
    requires all source files to be uniquely named, it might be good to
    use some kind of prefix for all metaspace files.  If you do not
    like that idea (we use it in the different GCs for example), I
    think at least a few of the files could be given more specific
    names (counter.hpp and settings.?pp for example).  Maybe prefix all
    files with "ms"?
    
5) No space between variable and increment operator (i ++ -> i++)

6) Enums should be named UpperCamelCase, as should their values
    (ALL_UPPER_CASE values are allowed according to the style guide,
    but I think most new code uses UpperCamelCase).
    
7) With c++ 14, I would like all enums to instead be enum classes,
    they are safer and better scoped, their underlying type can be
    specified and they have (to my knowledge) no downside except for
    eventual backports.

9) metaspace_test.?pp, I have to look more into this, but it does not
    feel right to mix in test this way, I guess it is hard to move though?

10) Constructor initialization lists use at least three different
     styles (placement of "," and ":") and while I personally like the
     Haskell style with comma first on the line, I think we should keep
     the style consistent with the rest of HotSpot.

Thanks,
Leo


More information about the hotspot-runtime-dev mailing list