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

Mon Apr 27 17:37:14 UTC 2020

Hi Martin,

I would definitely encourage enabling this on other platforms, but given the large amount of platform specific code I ran into for x86_64, I didn’t feel like I could enable it on other platforms without being able to do thorough testing. I skimmed at other platforms and it ”seems to work”, but it needs verification.

If you could sanity check PPC and S390, that would be fantastic. I’m talking to Stuart about AArch64 as well, so hopefully we can check that box too soon.

Thanks,
/Erik

> On 27 Apr 2020, at 18:35, Doerr, Martin <martin.doerr at sap.com> wrote:
> 
> 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
>>> 
> 