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

Tue Apr 28 10:39:09 UTC 2020

Hi Frederic,

On 2020-04-27 17:51, Frederic Parain wrote:
> 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.

Okay. I filed https://bugs.openjdk.java.net/browse/JDK-8243996 for this, 
and will split out that change to that RFE.

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

I presume this comment is about the new 
https://bugs.openjdk.java.net/browse/JDK-8243996 right?
If so, I will have a look at what configurations we need to cover better 
for that change.

> Thank you for addressing this technical debt and solving it cleanly.

You are welcome! Thank you for reviewing my code.

Thanks,
/Erik

> 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