8295486: Inconsistent constant field values observed during compilation #11861

TobiHartmann · 2023-01-05T13:26:11Z

We hit a "not monotonic" assert because the new type of a load from a stable final field is more narrow than the old type which contradicts the assumption that types should only go from TOP to BOTTOM during CCP:

old: narrowoop: java/lang/Integer:BotPTR:exact *
new: narrowoop: java/lang/Integer java.lang.Integer {0x000000062c41e548} ...

or

old: narrowoop: java/lang/Integer java.lang.Integer {0x000000062c41e538} ...
new: narrowoop: java/lang/Integer java.lang.Integer {0x000000062c41e548} ...

The problem is that a stable field can be (re-)initialized during compilation and since the value is not cached, contradicting types can be observed. In LoadNode::Value, we re-read the field value each time:

jdk/src/hotspot/share/opto/memnode.cpp

Lines 1994 to 1997 in 8723847

    
           if (!is_mismatched_access() && off != Type::OffsetBot && const_oop != NULL && const_oop->is_instance()) { 
        
             const Type* con_type = Type::make_constant_from_field(const_oop->as_instance(), off, is_unsigned(), memory_type()); 
        
             if (con_type != NULL) { 
        
               return con_type;

jdk/src/hotspot/share/opto/type.cpp

Lines 332 to 337 in 8723847

    
           const Type* Type::make_constant_from_field(ciInstance* holder, int off, bool is_unsigned_load, BasicType loadbt) { 
        
             ciField* field; 
        
             ciType* type = holder->java_mirror_type(); 
        
             if (type != NULL && type->is_instance_klass() && off >= InstanceMirrorKlass::offset_of_static_fields()) { 
        
               // Static field 
        
               field = type->as_instance_klass()->get_field_by_offset(off, /*is_static=*/true);

The same problem exists for loads from stable arrays:

jdk/src/hotspot/share/opto/memnode.cpp

Line 1923 in 8723847

    
           if (aobj != NULL && off_beyond_header && adr->is_AddP() && off != Type::OffsetBot) {

Caching the field value is not feasible as it would require a cache per ciInstance for all the fields and per ciArray for all the elements. Alternatively, we could keep track of the lookup and only do it once but that would also be lots of additional complexity for a benign issue.

Instead, I propose to skip verification during CCP when folding loads from stable fields. Non-stable, constant fields are not affected as null is a valid value for them and they would already be folded before CCP.

Thanks,
Tobias

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8295486: Inconsistent constant field values observed during compilation

Reviewers

Christian Hagedorn (@chhagedorn - Reviewer) ⚠️ Review applies to 32e3287e
Vladimir Kozlov (@vnkozlov - Reviewer)
Jatin Bhateja (@jatin-bhateja - Reviewer) ⚠️ Review applies to 048b273f
Vladimir Ivanov (@iwanowww - Reviewer) ⚠️ Review applies to 23344db9

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11861/head:pull/11861
$ git checkout pull/11861

Update a local copy of the PR:
$ git checkout pull/11861
$ git pull https://git.openjdk.org/jdk pull/11861/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11861

View PR using the GUI difftool:
$ git pr show -t 11861

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11861.diff

bridgekeeper · 2023-01-05T13:27:22Z

👋 Welcome back thartmann! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-01-05T13:30:01Z

@TobiHartmann The following label will be automatically applied to this pull request:

hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-01-05T13:33:40Z

Webrevs

chhagedorn

That looks reasonable, nice test!

openjdk · 2023-01-05T14:52:54Z

@TobiHartmann This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8295486: Inconsistent constant field values observed during compilation

Reviewed-by: chagedorn, kvn, jbhateja, vlivanov

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 39 new commits pushed to the master branch:

60c535d: 8301340: Make DirtyCardToOopClosure stack-allocated
b76a52f: 8301076: Replace NULL with nullptr in share/prims/
90ec19e: 8301068: Replace NULL with nullptr in share/jvmci/
419409b: 8301337: Remove unused os::_polling_page
d583767: 8301338: Identical branch conditions in CompileBroker::print_heapinfo
810c8a2: 8301170: perfMemory_windows.cpp add free_security_attr to early returns
33e653e: 8301448: [BACKOUT] CodeHeap has virtual methods that are not overridden
cdb4ba9: 8301326: Optimize compiler/uncommontrap/TestDeoptOOM.java test
9cc0171: 8301153: RISC-V: pipeline class for several instructions is not set correctly
633e291: 8301067: RISC-V: better error message when reporting unsupported satp modes
... and 29 more: https://git.openjdk.org/jdk/compare/64b25ea0b410542635b6d99a92ec290da47c85ce...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

TobiHartmann · 2023-01-05T15:08:01Z

Thanks, Christian!

vnkozlov

@vlivanov Do we have a mechanism to verify that Stable fields are initialized only once? Based on Tobias's test (which continuously modifies values) it does not work if it even exists. Then what is definition of Stable attribute then? We can't constant fold a field's value which can be changed (except from NULL to a value).

Tobias's comment in JBS about field point to different DirectMethodHandles concerns me.

For me the only valid solution is throw out compilation if it see change in stable filed's state. We have validation step in ciEnv::register_method(). May be create table of stable fields values we folded and verify during registration to make sure it did not change. It may not catch case in Tobias's test where the same values are rotated.

Note, one change from null to not-null is valid case. May be new code in PhaseCCP::verify_type() should check for initial NULL value before skipping following checks.

TobiHartmann · 2023-01-06T08:01:25Z

Unfortunately, there is no such mechanism. In the case of racy initializations, it can frequently happen that a stable field is written multiple times by different threads (see also my comments in JDK-8288970). This is expected and also mentioned in the C2 code:

jdk/src/hotspot/share/opto/compile.hpp

Line 266 in 775da84

    
           // In the case of @Stable, multiple writes are possible but may be assumed to be no-ops.

The definition of @Stable is basically that we can constant-fold any value once it's initialized and (semantically) it does not make a difference which value we choose.

If we go with your proposed solution of a table of field values, we can as well use that one as cache instead of bailing out from compilation, right?

jatin-bhateja

Nice test!, problem seems to be intermittent.

jatin-bhateja · 2023-01-06T07:32:07Z

src/hotspot/share/opto/phaseX.cpp

+  assert(!told->isa_int() || !tnew->isa_int() || told->isa_int()->_widen <= tnew->isa_int()->_widen, "widen increases");
+  assert(!told->isa_long() || !tnew->isa_long() || told->isa_long()->_widen <= tnew->isa_long()->_widen, "widen increases");


Minor correction, told->isa_int()->_widen <= tnew->isa_int()->_widen can be replaced by told->is_int()->_widen <= tnew->is_int()->_widen, preceding expression makes sure types are ints

jatin-bhateja · 2023-01-06T07:38:50Z

src/hotspot/share/opto/phaseX.cpp

+  if (FoldStableValues && n->is_Load() &&
+      ((atp->field() != NULL && atp->field()->is_stable()) ||
+       (adr_type->isa_aryptr() && adr_type->is_aryptr()->is_stable()))) {
+    return;


As, per specs Semantics are not clear if a @stable annotated field's values are changed multiple times, in your test case update thread explicitly changes the values of static stable fields. Change looks correct to skip over monotonicity constraint for this corner case

vnkozlov · 2023-01-06T18:13:12Z

The definition of @stable is basically that we can constant-fold any value once it's initialized and (semantically) it does not make a difference which value we choose.

Okay. The spec is really bizarre for @stable

If we go with your proposed solution of a table of field values, we can as well use that one as cache instead of bailing out from compilation, right?

Right. I prefer caching because the value will be consistent at least during compilation and it may allow some optimizations to happen. Imaging you read same stable field on separate paths and then compare constants.

iwanowww · 2023-01-06T18:39:09Z

I fully support caching of non-default values. (Not sure about default values yet: at least, it makes sense to cache them as well to make compilations reproducible.)

As an idea, the table could be built on top of existing dependency mechanism, but @Stable-related dependencies have to be dropped when migrated to the nmethod.

iwanowww · 2023-01-06T18:42:21Z

@vlivanov Do we have a mechanism to verify that Stable fields are initialized only once?

As Tobias already pointed out, there's no such support in place yet. Only a long-standing RFE to add it:
https://bugs.openjdk.org/browse/JDK-8024042

jatin-bhateja · 2023-01-06T18:53:56Z

The definition of @stable is basically that we can constant-fold any value once it's initialized and (semantically) it does not make a difference which value we choose.

Okay. The spec is really bizarre for @stable

If we go with your proposed solution of a table of field values, we can as well use that one as cache instead of bailing out from compilation, right?

Right. I prefer caching because the value will be consistent at least during compilation and it may allow some optimizations to happen. Imaging you read same stable field on separate paths and then compare constants.

If value keep changing during CCP then it will delay the convergence. Thus a convergence will be reached only for a stable value, which is what caching will also achieve. Stable field value should be added to method dependencies such that compilation becomes invalid if stable value changes later on.

Agree with @iwanowww suggestion

iwanowww · 2023-01-06T19:15:27Z

Stable field value should be added to method dependencies such that compilation becomes invalid if stable value changes later on.

It's too much for @stable. It's allowed to observe stale values when concurrent modifications happen. It would require dependency checking on every @Stable field or array element update which is infeasible for arrays (all array stores have to be intercepted).

On the other hand, caching a single value which is used across the whole compilation has only performance impact (whether non-null value is observed or not).

jatin-bhateja · 2023-01-07T03:34:28Z

Stable field value should be added to method dependencies such that compilation becomes invalid if stable value changes later on.

It's too much for @stable. It's allowed to observe stale values when concurrent modifications happen. It would require dependency checking on every @Stable field or array element update which is infeasible for arrays (all array stores have to be intercepted).

On the other hand, caching a single value which is used across the whole compilation has only performance impact (whether non-null value is observed or not).

Agree, but there should be a way for method to deoptimize if stable field value changes past method registration. Only full proof solution is to enforce that stable field value does not change after single initialization apart from null to non-null transitions.

TobiHartmann · 2023-01-09T13:20:04Z

Vladimirs, Jatin, thanks for the reviews and discussion!

The question is what we want to achieve with this patch:

Prevent C2 from crashing / asserting when observing inconsistent field values
Prevent C2 from applying optimizations based on inconsistent field values
Prevent C2 from constant folding stale values

I think my proposed patch achieves 1) which is also the only issue we ever observed in real code: Racy initialization of method handles for indy string concat in core libraries code. The verification code proposed by (JDK-8024042) would catch this. Constant folding a stale value does not matter here, semantically, we can use any of the equivalent method handles that we observe. It just confuses CCP verification. Convergence of CCP is also not an issue because we don't update a node's type indefinitely but only if the type of a relevant input node changed.

Regarding 2): Couldn't any such wrong/undesired behaviour happen with execution in the interpreter as well? The only difference would be that optimized C2 compiled code would always behave that way. But according to the specification for @Stable, that's okay:

jdk/src/java.base/share/classes/jdk/internal/vm/annotation/Stable.java

Lines 74 to 80 in 5c8c67c

    
            * It is (currently) undefined what happens if a field annotated as stable 
        
            * is given a third value (by explicitly updating a stable field, a component of 
        
            * a stable array, or a final stable field via reflection or other means). 
        
            * Since the HotSpot VM promotes a non-null component value to constant, it may 
        
            * be that the Java memory model would appear to be broken, if such a constant 
        
            * (the second value of the field) is used as the value of the field even after 
        
            * the field value has changed (to a third value).

Also, doesn't the above specification mean that all non-synchronized initializations of stable fields can lead to undefined behavior?

Imaging you read same stable field on separate paths and then compare constants.

That will also happen with interpreted code when the stable field is written between the two reads.

The only complete solution would be 3), which, as Vladimir I. already pointed out, does not seem feasible given that we would need to intercept all writes to stable fields and array elements.

I'm not against caching but I'm wondering how much sense it makes to apply an expensive and complex (partial) fix to C2 while C1 and the interpreter are still affected. Shouldn't the remaining issues be fixed in Java code if undefined/unexpected behavior is ever observed?

iwanowww · 2023-01-09T18:17:58Z

IMO we should focus solely on C2 where aforementioned inconsistencies trigger assertion failures and (rarely) break compilation replay. (The rest is classified as user errors according to @Stable contract.) Caching a single value (either null or non-null) for the duration of the compilation addresses both problems.

iwanowww · 2023-01-09T18:26:54Z

but there should be a way for method to deoptimize if stable field value changes past method registration.

It is not required for proper @Stable support and, moreover, doesn't make much sense considering how the API is shaped.

@Stable is not part of public API (has no effect in non-trusted code) for a reason: it deliberately doesn't try to enforce the invariants it depends on and leaves users to struggle with the consequences without providing any help. So, caution is needed when it is used.

iwanowww · 2023-01-09T18:35:05Z

FTR there have been multiple attempts to build a story for "lazy final" fields:

vnkozlov · 2023-01-09T19:58:13Z

IMO we should focus solely on C2 where aforementioned inconsistencies trigger assertion failures and (rarely) break compilation replay. (The rest is classified as user errors according to @Stable contract.) Caching a single value (either null or non-null) for the duration of the compilation addresses both problems.

+1

C2 generated code is "final" (C1 code and Interpreter are executed only at initial stage with exception of deoptimization).

TobiHartmann · 2023-01-10T13:43:44Z

Okay, I implemented caching of stable values. Please let me know what you think.

vnkozlov

Nice work.

iwanowww · 2023-01-10T18:36:46Z

src/hotspot/share/ci/ciEnv.hpp

+      ciConstant value() const { return _value; }
+  };
+
+  GrowableArray<StableValue>* _stable_values; // Cache of stable values


Aren't ciInstance and ciArray provide a better place to cache field/element values (logic can be encapsulated in ciInstance::field_value() and ciArray::element_value())?

And it would seamlessly cover all possible cases, like static final fields (which can be concurrently updated through Reflection API or Unsafe).

But wouldn't that require duplication of the ciEnv::check_stable_value logic and having a GrowableArray per ciObject instance?

Regarding covering more cases like static final fields: I intentionally omitted them because I don't think it's possible to delay constant folding of such loads until CCP (since null is a valid value) and the lookup is done only once during (I)GVN. The regression test includes a corresponding case (see testFinal). Even with unsafe accesses, if the field holder is not known at parse time, the access is conservatively marked as mismatched and thus no constant folding is attempted later.

But wouldn't that require duplication of the ciEnv::check_stable_value logic and having a GrowableArray per ciObject instance?

Are you primarily concerned about footprint? The field can be lazily initialized to reduce possible effects.
check_stable_value still can be shared (e.g., placed on ciObject).

Regarding covering more cases like static final fields: I intentionally omitted them because I don't think it's possible to delay constant folding of such loads until CCP

If you want to limit the current patch to stable case only, that's fine with me. But keep in mind that we still want to uniformly treat all cases where field/array element values are considered constant. Concurrent field modifications introduce non-determinism into compilation process which we would like to avoid. So, even if we don't cover final fields right away, let's prepare for a future enhancement which will cover that. In that respect, stable references in names are misleading.

My concern is that putting the logic in ciObject adds a new field to all subclass instances while it's only rarely needed. Given that a large number of these objects are created during compilation, that adds to footprint even if the cache is initialized lazily.

I still gave this a try and another issue is that using ciConstant in ciObject creates a circular dependency (invalid use of incomplete type) because ciConstant depends on ciObject. We could of course duplicate the logic and put things both into ciInstance and ciArray but then a side table in ciEnv seems preferable to me.

Until proven otherwise, I would like to limit the current patch to stable cases because I don't think there are any issues for non-stable cases. I can rename the code from "stable" to "constant" but I'm not sure how much sense that makes given it's all limited to and guarded by FoldStableValues. What do you think?

Personally, I'm not concerned about footprint increase from a field on ciObject class. Those represent oop constants observed by JITs and their count should be modest.

Speaking of relation between ciConstant and ciObject, ciConstant.hpp only refers to ciObject*, so the cycle can be broken by prepending class ciObject; in ciConstant.hpp.

I don't think there are any issues for non-stable cases.
I'm fine with addressing only stable cases with the current patch for now, but I don't agree there's nothing to improve for non-stable cases.

Concurrent field modifications may seem benign, but our previous experience shows that it's safer to work with a consistent view through CI (when it is feasible/possible).
(There's a relevant RFE filed: https://bugs.openjdk.org/browse/JDK-8294616) And it helps improve compilation replay along the way. And it's not specific to C2.

So, IMO in the longer term CI is the right place to fix it (irrespective whether stable or all constant cases are handled). It's a bit strange to see the new checks added in opto/type.cpp (irrespective of where the cache is located).

Speaking of relation between ciConstant and ciObject, ciConstant.hpp only refers to ciObject*, so the cycle can be broken by prepending class ciObject; in ciConstant.hpp.

That won't help. All ci* classes, including ciObject, are already forward declared in ciClassList.hpp which is included in ciConstant.hpp. The following usages in ciConstant require a complete ciObject type:

jdk/src/hotspot/share/ci/ciConstant.hpp

Line 115 in c5c8c06

return as_object()->is_null_object();

jdk/src/hotspot/share/ci/ciConstant.hpp

Line 134 in c5c8c06

return as_object()->is_loaded();

Another issue is that ciConstant.hpp includes ciNullObject.hpp which inherits from ciObject. I worked around both issues by moving code and includes around.

I moved the code to ciObject, as you proposed, and extended it to include final fields as well. Let me know what you think.

Testing now revealed a new "Not monotonic" assert that seems to be triggered by caching non-stable final fields. I'll investigate but I'm not able to reproduce it yet.

jatin-bhateja · 2023-01-12T07:13:01Z

Vladimirs, Jatin, thanks for the reviews and discussion!

The question is what we want to achieve with this patch:

Prevent C2 from crashing / asserting when observing inconsistent field values

Prevent C2 from applying optimizations based on inconsistent field values

Prevent C2 from constant folding stale values

I think my proposed patch achieves 1) which is also the only issue we ever observed in real code: Racy initialization of method handles for indy string concat in core libraries code. The verification code proposed by (JDK-8024042) would catch this. Constant folding a stale value does not matter here, semantically, we can use any of the equivalent method handles that we observe. It just confuses CCP verification. Convergence of CCP is also not an issue because we don't update a node's type indefinitely but only if the type of a relevant input node changed.

Regarding 2): Couldn't any such wrong/undesired behaviour happen with execution in the interpreter as well? The only difference would be that optimized C2 compiled code would always behave that way. But according to the specification for @Stable, that's okay:

jdk/src/java.base/share/classes/jdk/internal/vm/annotation/Stable.java

Lines 74 to 80 in 5c8c67c

* It is (currently) undefined what happens if a field annotated as stable

* is given a third value (by explicitly updating a stable field, a component of

* a stable array, or a final stable field via reflection or other means).

* Since the HotSpot VM promotes a non-null component value to constant, it may

* be that the Java memory model would appear to be broken, if such a constant

* (the second value of the field) is used as the value of the field even after

* the field value has changed (to a third value).

Also, doesn't the above specification mean that all non-synchronized initializations of stable fields can lead to undefined behavior?

Imaging you read same stable field on separate paths and then compare constants.

That will also happen with interpreted code when the stable field is written between the two reads.

The only complete solution would be 3), which, as Vladimir I. already pointed out, does not seem feasible given that we would need to intercept all writes to stable fields and array elements.

I'm not against caching but I'm wondering how much sense it makes to apply an expensive and complex (partial) fix to C2 while C1 and the interpreter are still affected. Shouldn't the remaining issues be fixed in Java code if undefined/unexpected behavior is ever observed?

Thanks @TobiHartmann for explanations. Caching looks correct. First intercept of constant field folding is dring parsing when a field access is made and later on gvn folds the constant field loads. Similar change could be done for C1

jdk/src/hotspot/share/c1/c1_GraphBuilder.cpp

Line 1797 in af8d3fb

ciConstant field_value = field->constant_value_of(const_oop);

But given that problem is only seen during aggressive optimization like CCP I agree with @iwanowww's suggestion of restricting the scope of fix to C2.

iwanowww · 2023-01-20T18:52:04Z

As it is now, it doesn't help static/instance fields, but it can be improved. Something for a follow-up cleanup.
Could you elaborate? Final static/instance fields are handled as well (ciField::constant_value_of -> ciInstance::field_value -> ciInstance::field_value_impl). Do you mean non-final fields?

Not necessarily. I spotted the following for static final fields:

ciConstant ciField::constant_value() {
  ...
  if (_constant_value.basic_type() == T_ILLEGAL) {
    // Static fields are placed in mirror objects.
    VM_ENTRY_MARK;
    ciInstance* mirror = CURRENT_ENV->get_instance(_holder->get_Klass()->java_mirror());
    _constant_value = mirror->field_value_impl(type()->basic_type(), offset());
  }

As it is now, you can't avoid the state transition when doing field_value_impl() call.
You could reshape it into:

  if (_constant_value.basic_type() == T_ILLEGAL) {
    // Static fields are placed in mirror objects.
    ciInstance* mirror = _holder->java_mirror();
    _constant_value = mirror->field_value_impl(type()->basic_type(), offset());
  }

... and then put a call to check the cache first before trying to load a value from the field. There should be a VM_ENTRY_MARK in field_value_impl(), but the check can be placed before it. Then ciInstance::field_value() won't need GUARDED_VM_ENTRY when calling into field_value_impl().

iwanowww · 2023-01-20T18:52:25Z

I reproduced it. The problem is the new call to verify_type that I added to PhaseCCP::verify_analyze.

Nice catch!

src/hotspot/share/ci/ciArray.cpp

src/hotspot/share/ci/ciObject.cpp

TobiHartmann · 2023-01-24T13:44:02Z

Thanks again, Vladimir. I updated the change according to your suggestions.

iwanowww

Overall, looks good!

src/hotspot/share/ci/ciObject.cpp

TobiHartmann · 2023-01-25T07:27:33Z

Thanks again, Vladimir. I incorporated both your suggestions.

iwanowww

Looks good.

TobiHartmann · 2023-01-25T19:02:43Z

Thanks Vladimir. @vnkozlov, @chhagedorn, @jatin-bhateja are you okay with the latest version as well?

vnkozlov

I am fine with moving caching to CI.

As I see (may be wrong) you allow C2 to see all cached values which may introducing inconsistency in generated code. I am not sure how PhaseCPP::verify_type() works in such case.

src/hotspot/share/ci/ciObject.hpp

TobiHartmann · 2023-01-26T06:12:11Z

As I see (may be wrong) you allow C2 to see all cached values which may introducing inconsistency in generated code. I am not sure how PhaseCPP::verify_type() works in such case.

I'm not sure if I understand your concern, what do you mean by "all cached values"? The cache will only contain values observed during the current compilation (it's lifetime is limited by the lifetime of ciObject which is compile task specific) and it will ensure that the same value is used for constant folding of a specific field or array element.

vnkozlov · 2023-01-26T19:23:27Z

As I see (may be wrong) you allow C2 to see all cached values which may introducing inconsistency in generated code. I am not sure how PhaseCPP::verify_type() works in such case.

I'm not sure if I understand your concern, what do you mean by "all cached values"? The cache will only contain values observed during the current compilation (it's lifetime is limited by the lifetime of ciObject which is compile task specific) and it will ensure that the same value is used for constant folding of a specific field or array element.

After reading Vladimir's and your discussion and looking on code I see that you check value in cache before adding it to cache. So my concern is mute. But why do you need GrowableArray in such case? That confused me because I first thought you cache all values.

TobiHartmann · 2023-01-27T06:11:25Z

But why do you need GrowableArray in such case? That confused me because I first thought you cache all values.

Because there's only a very limited number of constant fields per compilation, so I thought a simple array is good enough. Do you prefer a hash map (Dict or something) instead?

vnkozlov · 2023-01-27T17:49:02Z

But why do you need GrowableArray in such case? That confused me because I first thought you cache all values.

Because there's only a very limited number of constant fields per compilation, so I thought a simple array is good enough. Do you prefer a hash map (Dict or something) instead?

My mistake. I thought you collected values per field. But you collected all constant fields per object. Good. GrowableArray is perfectly fine for that. Finally I got this ;^)

One last question. Do I understand correctly that static fields constant value is already handled by ciField::constant_value()?

TobiHartmann · 2023-01-30T06:01:05Z

Thanks again, Vladimir.

One last question. Do I understand correctly that static fields constant value is already handled by ciField::constant_value()?

Yes, for example for C2, final static fields are handled via Type::make_constant_from_field -> ciField::constant_value -> ciInstance::field_value_impl (on the mirror object) which will then check the cache via check_constant_value_cache.

vnkozlov

Good. Thank you for answering all my questions.

TobiHartmann · 2023-02-01T11:14:00Z

Thanks, Vladimir!

TobiHartmann · 2023-02-01T11:14:08Z

/integrate

openjdk · 2023-02-01T11:15:50Z

Going to push as commit cae577a.
Since your change was applied there have been 52 commits pushed to the master branch:

969f6a3: 8301093: C2 fails assert(ctrl == kit.control()) failed: Control flow was added although the intrinsic bailed out
2a8ae2f: 8300256: C2: vectorization is sometimes skipped on loops where it would succeed
ef0d0a7: 8301402: os::print_location gets is_global_handle assert
4f6f3cc: 8301446: Remove unused includes of gc/shared/genOopClosures
a0aed9b: 8301459: Serial: Merge KeepAliveClosure into FastKeepAliveClosure
d269ebb: 8301570: Test runtime/jni/nativeStack/ needs to detach the native thread
8164cfb: 8300696: [AIX] AttachReturnError fails
09bfbf8: 8300909: Update com/sun/jndi/dns/Test6991580.java manual test instruction
6beadbb: 8293519: deprecation warnings should be emitted for uses of annotation methods inside other annotations
4bef233: 8301549: Fix comment about ClassCircularityError
... and 42 more: https://git.openjdk.org/jdk/compare/64b25ea0b410542635b6d99a92ec290da47c85ce...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-02-01T11:16:04Z

@TobiHartmann Pushed as commit cae577a.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

TobiHartmann added 2 commits January 5, 2023 13:37

8295486: Inconsistent constant field values observed during compilation

2ee34bc

Updated copyright

32e3287

openjdk bot added the rfr Pull request is ready for review label Jan 5, 2023

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Jan 5, 2023

chhagedorn approved these changes Jan 5, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 5, 2023

vnkozlov reviewed Jan 5, 2023

View reviewed changes

jatin-bhateja reviewed Jan 6, 2023

View reviewed changes

Caching of stable values

048b273

vnkozlov approved these changes Jan 10, 2023

View reviewed changes

iwanowww reviewed Jan 10, 2023

View reviewed changes

iwanowww reviewed Jan 20, 2023

View reviewed changes

src/hotspot/share/ci/ciArray.cpp Outdated Show resolved Hide resolved

src/hotspot/share/ci/ciObject.cpp Outdated Show resolved Hide resolved

Check cache before state transition

518e31c

Merge branch 'master' into JDK-8295486

1272bf5

iwanowww approved these changes Jan 24, 2023

View reviewed changes

src/hotspot/share/ci/ciObject.cpp Outdated Show resolved Hide resolved

src/hotspot/share/ci/ciObject.cpp Show resolved Hide resolved

Assert to catch duplicates and GrowableArray init

23344db

iwanowww approved these changes Jan 25, 2023

View reviewed changes

vnkozlov reviewed Jan 25, 2023

View reviewed changes

src/hotspot/share/ci/ciObject.hpp Outdated Show resolved Hide resolved

Switched order of fields

2927df3

TobiHartmann added 2 commits January 26, 2023 07:13

Fixed typo

d07ee38

Forgot to change order in constructor

b78437a

Merged with master

c46e85f

vnkozlov approved these changes Jan 31, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Feb 1, 2023

openjdk bot closed this Feb 1, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 1, 2023

	if (!is_mismatched_access() && off != Type::OffsetBot && const_oop != NULL && const_oop->is_instance()) {
	const Type* con_type = Type::make_constant_from_field(const_oop->as_instance(), off, is_unsigned(), memory_type());
	if (con_type != NULL) {
	return con_type;

	const Type* Type::make_constant_from_field(ciInstance* holder, int off, bool is_unsigned_load, BasicType loadbt) {
	ciField* field;
	ciType* type = holder->java_mirror_type();
	if (type != NULL && type->is_instance_klass() && off >= InstanceMirrorKlass::offset_of_static_fields()) {
	// Static field
	field = type->as_instance_klass()->get_field_by_offset(off, /is_static=/true);

		assert(!told->isa_int() \|\| !tnew->isa_int() \|\| told->isa_int()->_widen <= tnew->isa_int()->_widen, "widen increases");
		assert(!told->isa_long() \|\| !tnew->isa_long() \|\| told->isa_long()->_widen <= tnew->isa_long()->_widen, "widen increases");

8295486: Inconsistent constant field values observed during compilation #11861

8295486: Inconsistent constant field values observed during compilation #11861

Uh oh!

Conversation

TobiHartmann commented Jan 5, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jan 5, 2023

Uh oh!

openjdk bot commented Jan 5, 2023

Uh oh!

mlbridge bot commented Jan 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

chhagedorn left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Jan 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TobiHartmann commented Jan 5, 2023

Uh oh!

vnkozlov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TobiHartmann commented Jan 6, 2023

Uh oh!

jatin-bhateja left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vnkozlov commented Jan 6, 2023

Uh oh!

iwanowww commented Jan 6, 2023

Uh oh!

iwanowww commented Jan 6, 2023

Uh oh!

jatin-bhateja commented Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iwanowww commented Jan 6, 2023

Uh oh!

jatin-bhateja commented Jan 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TobiHartmann commented Jan 9, 2023

Uh oh!

iwanowww commented Jan 9, 2023

Uh oh!

iwanowww commented Jan 9, 2023

Uh oh!

iwanowww commented Jan 9, 2023

Uh oh!

vnkozlov commented Jan 9, 2023

Uh oh!

TobiHartmann commented Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

TobiHartmann commented Jan 5, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 5, 2023 •

edited

Loading

openjdk bot commented Jan 5, 2023 •

edited

Loading

vnkozlov left a comment •

edited

Loading

jatin-bhateja left a comment •

edited

Loading

jatin-bhateja commented Jan 6, 2023 •

edited

Loading

jatin-bhateja commented Jan 7, 2023 •

edited

Loading

TobiHartmann commented Jan 10, 2023 •

edited

Loading