RFR: 8251158: Implementation of JEP 387: Elastic Metaspace

[Thomas Stüfe]

Hi Leo,

thanks a lot for your review work!

Remarks inline.

On Thu, Aug 20, 2020 at 12:24 PM Leo Korinth <leo.korinth at oracle.com> wrote:

> 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.
>

(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.
>

Okay.


>
> Thanks,
> Leo
>

Thanks, Thomas