RFR: 8251158: Implementation of JEP 387: Elastic Metaspace
thomas.stuefe at gmail.com
Fri Aug 21 05:04:36 UTC 2020
thanks a lot for your review work!
On Thu, Aug 20, 2020 at 12:24 PM Leo Korinth <leo.korinth at oracle.com> wrote:
> 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.
(1-3) Yes, I may be a secret C programmer inside. I find simple structs
with public members and without ctors to be easier to read and less
boilerplate for structured output data. But no problem, I will fix this.
When in Rome.
> 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"?
I thought this myself. Especially since currently it's really inconsistent.
I refrained from doing it in order to keep the patches smaller but that
ship may have long sailed :)
I think something up.
> 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).
5, 6: Ok
> 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.
I would love to use them, especially for MetaspaceType/MetadataType, but
cannot use them for all cases. Enum classes miss the ability to iterate
over values. Which is really a pity, how could they miss that.
But I will use them for Metachunk::state, I thought so myself already. I
love the fact that one can specify enum size explicitly with enum classes.
> 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?
Yes, unfortunately. I could hide those two classes away in some other file,
but that seems not like a great improvement.
They are used by whitebox APIs as well as gtests, and because of the former
they are hard to move.
I have some smaller improvements in mind after we get this patch out of the
door though, among others this:
https://bugs.openjdk.java.net/browse/JDK-8252132, which would do away at
least with MetaspaceTestArena.
> 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.
More information about the hotspot-gc-dev