RFR: 8241825: Make compressed oops and compressed class pointers independent on x86_64

[Doerr, Martin]

Hi Erik,

I think this change makes sense (not a review).

But I guess this was a x86 specific limitation. I can't see why we're not setting COMPRESSED_CLASS_POINTERS_DEPENDS_ON_COMPRESSED_OOPS to false on other platforms.
I think it should work at least on PPC64 and s390. But let me double-check.
(Haven't checked others.)

Best regards,
Martin


> -----Original Message-----
> From: hotspot-dev <hotspot-dev-bounces at openjdk.java.net> On Behalf Of
> Frederic Parain
> Sent: Montag, 27. April 2020 17:51
> To: Erik Österlund <erik.osterlund at oracle.com>
> Cc: hotspot-dev at openjdk.java.net
> Subject: Re: RFR: 8241825: Make compressed oops and compressed class
> pointers independent on x86_64
> 
> Erik,
> 
> Thank you for taking care of this issue.
> 
> > On Apr 27, 2020, at 11:28, Erik Österlund <erik.osterlund at oracle.com>
> wrote:
> >
> > Hi Frederic,
> >
> > I hear your concern about hardcoded offsets making the JVM more fragile.
> >
> > First of all, let me assure you that you would not be able to run hello world
> if the offsets ever were to be
> > wrong. They are verified during bootstrapping in
> JavaClasses::check_offsets(), so you have to be quite skilled
> > to manage to break anything without immediately noticing it. You literally
> can't have run a single test or java -version
> > without immediately crashing. This makes this significantly less fragile. So I
> don't think I believe that this is fragile.
> 
> I agree. I was planning to remove this code in 16, as part of the old layout
> code removal,
> but it can still be useful for some time.
> 
> >
> > Having said that, I do agree it could be annoying and a pain to maintain and
> update the layout code if such changes
> > are occasionally incompatible with the random selection of hardcoded
> offsets, forcing new awkward updates of said offsets,
> > and that hardcoding things in general is undesirable, compared to a more
> solid solution.
> 
> I worked around the problem by isolating these classes so they don’t impact
> the regular
> cases of field layout, but it was just a work around. Completely removing
> hardcoded offsets
> is a cleaner and even more robust solution.
> 
> > I had a look at an approach for removing hardcoded offsets completely. It
> looks something like this:
> >
> > http://cr.openjdk.java.net/~eosterlund/8241825/webrev.00..not_01/
> >
> > The change is a bit involved and complicated though. Because of that, I
> would prefer to push it as a follow-up RFE
> > focused on removing hardcoded offsets, if you are okay with that. This
> change is already quite tricky, and I think
> > these things ought to be separate.
> 
> As your changes linked above touch the VM init sequence (always a tricky
> area),
> I think dedicating a specific RFE for them is a good idea.
> 
> >
> > The fix involves splitting allocation and code generation of the interpreter
> (due to dependencies with verification code in i2i adapters
> > generated during method loading, requiring the interpreter to be allocated
> early), computing offsets of jlr.Reference early during
> > resolution of well known classes (as is done for java_lang_String and
> java_lang_Class), because it needs offsets to remove the
> > discovered and referent fields from oop maps before its subclasses are
> initialized, and moving initialization (and generation)
> > of the interpreter until after offsets have been computed (no longer
> having the dependency to method loading and linking).
> > This way, all offsets have always been computed (or loaded by CDS) before
> they are needed, without any need for hardcoding.
> >
> > What do you think?
> 
> Sounds like a good plan. I’ve read your changes, and they look fine to me.
> However, I would be more comfortable with extended testing to cover all
> the
> configurations (+/-CDS, templateInterpreter/cppInterpreter, C2/Graal) to be
> sure we didn’t miss a dependency somewhere.
> 
> Thank you for addressing this technical debt and solving it cleanly.
> 
> Regards,
> 
> Fred
> 
> 
> >
> > On 2020-04-24 16:45, Frederic Parain wrote:
> >>
> >>> On Apr 24, 2020, at 10:36, coleen.phillimore at oracle.com wrote:
> >>>
> >>>
> >>>
> >>> On 4/24/20 10:16 AM, Erik Österlund wrote:
> >>>> Hi Frederic,
> >>>>
> >>>> Thanks for reviewing this.
> >>>>
> >>>> On 2020-04-24 15:28, Frederic Parain wrote:
> >>>>> Hi Erik,
> >>>>>
> >>>>> Why did you removed the code handling classes with hard coded
> layouts?
> >>>>> This code was there to provide the freedom to play with layout
> algorithms
> >>>>> without having to deal with these special cases. Without this code,
> any
> >>>>> modification is now constrained by these special classes.
> >>>> Good question. Since I started using the klass gap for fields, even when
> compressed oops is off, dealing with the
> >>>> hardcoded offsets and layouts really became quite awkward, to the
> extent that I started wondering why do we even have
> >>>> special layouts for these things anyway. I don't like special things.
> Treating it like any other object seemed far
> >>>> easier, than matching the special special hardcoded layouts with the
> hardcoded offsets.
> >>>>
> >>>> I wouldn't say that the whole layout algorithm is constrained by the fact
> that there are hardcoded offsets.
> >>>> I would say that it's just a matter of whoever changes the layout
> algorithms such that the hardcoded offsets
> >>>> are no longer the same, is just gonna have to update the offset to
> whatever the correct offsets are with the
> >>>> new algorithm. And if you look at the classes where we actually do
> hardcode the offsets, they are rather vanilla
> >>>> with relatively few ways you could change the layout other than
> changing it around for the sake of it. Especially
> >>>> primitive box objects. My fantasy is quite limited how we expect a new
> layout algorithm is gonna do anything else
> >>>> there really.
> >>>>
> >>>> Philosophically, I really think it is up to the person hardcoding an offset
> to know what the offset is going to
> >>>> be, ratherthan having completely different special (and worse) layouts
> produced for certain objects, just to
> >>>> satisfy the expectations of the hardcoded offsets.Especially if the result
> is pretty bad memory waste in for
> >>>> example Integer box objects, that can be quite plentyful in an
> application.
> >>> We used to have a lot more special hardcoded layouts but most have
> been removed. The reason these few classes are hardcoded is because the
> interpreter generates code using these offsets before
> JavaClasses::compute_offsets is run.  We tried to get rid of all of them and
> maybe there's a way to indirectly refer to them in the generated code so we
> can do so in the future.
> >> The proposed changes make the whole system brittle. Restoring the
> special handling of these classes during
> >> layout computation or making the generated code able to deal with
> dynamically computed offsets would be a
> >> more robust solution.
> >>
> >>
> >>>> The code to figure out the right offsets if a very small amount of code,
> compared to the rather large amount of
> >>>> code to create special layouts and syncing the special hardcoded layout
> code with the hardcoded offsets. That
> >>>> is why my patch has a net negative amount of code.
> >>>>
> >>>> Philosophically I also don't like having "special" layouts because we
> don't trust our ability to hardcode some
> >>>> offset to the right value. Almost all "special" things are bad and create
> unnecessary complications, and hence
> >>>> should have very good motivation for being allowed to be special. In
> this case, I really could not see any reason
> >>>> why these layouts would be special, unless it was somehow super hard
> to figure out what the hardcoded offsets would be.
> >>>> But for the classes we currently need hardcoded offsets, the
> calculations are trivial. So I don't think there is
> >>>> enough motivation to have special (bad) layouts for these objects for
> that reason. It only makes things harder than
> >>>> it needs to be, and makes the layout worse.
> >>>>
> >>>>> And I’d prefer to keep the dispatch in
> FieldLayoutBuilder::build_layout()
> >>>>> because we have more cases to handle there for the Valhalla project
> (the
> >>>>> layout algorithm for inline types is different than the one in
> >>>>> compute_regular_layout()).
> >>>> Okay. There was no need for the dispatching any more since there was
> only the normal layout and nothing
> >>>> else. But I can put that code back into the compute_regular_layout()
> function if that makes it easier for you
> >>>> in Valhalla, and have FieldLayoutBuilder::build_layout() just call that
> function. Will update in the next webrev.
> >> Thank you,
> >>
> >> Fred
> >>
> >>>>
> >>>>> Regards,
> >>>>>
> >>>>> Fred
> >>>>>
> >>>>>
> >>>>>> On Apr 24, 2020, at 04:17, Erik Österlund
> <erik.osterlund at oracle.com> wrote:
> >>>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>> Today, the use of compressed class pointers assumes the use of
> compressed oops.
> >>>>>> This patch loosens up the relationship between compressed oops
> and compressed
> >>>>>> class pointers, so that compressed class pointers can be used
> without compressed
> >>>>>> oops. This benefits for example ZGC that wants compressed class
> pointers, but
> >>>>>> not compressed oops, effectively giving ZGC 4 bytes smaller objects.
> >>>>>>
> >>>>>> Much of the complexity of the change is that r12 used to be used by
> compressed
> >>>>>> class pointers as some kind of semi-fast temp register that one
> would restore
> >>>>>> to the compressed oops heap base (or zero). That made it
> effectively a slightly
> >>>>>> optimized spilling mechanism used to find a temp register. I replaced
> that
> >>>>>> mechanism with a plain old normal temp register that you pass in as
> a parameter.
> >>>>>> The bad news is that I had to find available temp registers in a lot of
> places.
> >>>>>> The good news is that almost always were there temp registers
> available for free,
> >>>>>> and hence the new temp register is even faster than the old
> optimized spilling
> >>>>>> mechanism. Because we almost never need any spilling at all in the
> contexts where
> >>>>>> this is used.
> >>>>>>
> >>>>>> Since I want the 4 new bytes to actually make objects smaller, I
> poked the new
> >>>>>> layout code a bit so that the klass gap is made available for fields.
> That used
> >>>>>> to be made available only with compressed oops enabled, due to
> restrictions in
> >>>>>> the layout code. Our new layout code does not have such
> restrictions, and so
> >>>>>> I will make the 4 bytes available for fields when the new layout code
> is used
> >>>>>> and compressed class pointers are used.
> >>>>>>
> >>>>>> Now I'm only fixing this for HotSpot x86_64. Ideally the use of
> compressed oops
> >>>>>> and compressed class pointers should not be entangled in other
> platforms and
> >>>>>> Graal. But that is beyond the scope of this change, and I will let them
> behave
> >>>>>> the way that they used to, to be potentially fixed later.
> >>>>>>
> >>>>>> Bug:
> >>>>>> https://bugs.openjdk.java.net/browse/JDK-8241825
> >>>>>>
> >>>>>> Webrev:
> >>>>>> http://cr.openjdk.java.net/~eosterlund/8241825/webrev.00/
> >>>>>>
> >>>>>> Testing:
> >>>>>> hs-tier1-7
> >>>>>>
> >>>>>> Thanks,
> >>>>>> /Erik
> >