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