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

[Frederic Parain]

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
>