[14] RFR(XS): 8227236: assert(singleton != __null && singleton != declared_interface) failed

[Vladimir Ivanov]

I'm not sure how practical would it be to provide a consistent snapshot 
of the entire world through CI. I agree that it's ugly when CI exposes 
contradictory info, but compilers can't fully trust the info anyway 
(irrespective of whether it is globally consistent or not) and all the 
observations should be validated either using nmethod dependencies or 
runtime checks in generated code.

Speaking of the particular problem, there is a set of ciInstanceKlass 
instances which are shared across all compilations and there's no good 
place to update the cached state once they are instantiated. So, on CI 
level caching is disabled for them and queries always go into runtime.

Frankly speaking, I'd prefer to see shared classes go away: it looks 
like a pure optimization with questionable results. But there may be 
some bootstrapping subtleties (e.g., see ciObjectFactory::initialize()) 
I'm missing.

Best regards,
Vladimir Ivanov

PS: this particular bug was exposed after Iterator was turned into a 
well-known [1] class (and hence became shared).

[1] http://hg.openjdk.java.net/jdk/jdk/rev/a0d4e61acb6b#l1.8

On 14/08/2019 13:49, Igor Veresov wrote:
> It’d be nice if CI provided a consistent snapshot of the world. Having inconsistencies like that is a bit scary.
> Is it because we don’t want to eagerly snapshot the whole hierarchy of the given class?
> 
> igor
> 
> 
> 
>> On Aug 13, 2019, at 3:36 PM, dean.long at oracle.com wrote:
>>
>> Looks OK, but what's the harm in memoizing for shared classes?
>>
>> dl
>>
>> On 8/13/19 8:40 AM, Vladimir Ivanov wrote:
>>> http://cr.openjdk.java.net/~vlivanov/8227236/webrev.00/
>>> https://bugs.openjdk.java.net/browse/JDK-8227236
>>>
>>> There's a race between ciInstanceKlass::nof_implementors()/implementor()
>>> and class loading for shared classes which may manifest as
>>> inconsistency between consecutive calls to nof_implementors() and
>>> implementor() (ciInstanceKlass::implementor() doesn't cache
>>> InstanceKlass::implementor() for shared classes [1]). That's what
>>> triggers the assert: the check sees declared_interface->nof_implementors() == 1,
>>> but concurrent class loading introduces new implementor class before the
>>> assert. Assert hits singleton == declared_interface case since
>>> declared_interface->implementor() == declared_interface when
>>> declared_interface->nof_implementors() > 1 [2].
>>>
>>> The race has been there long before JDK-6986483 [3] (same sequence of
>>> nof_implementors()/implementor() calls and the assert in C1 code), but
>>> it seems recent JDK changes made it more likely to occur.
>>>
>>> Proposed fix is to check for unique implementor using a single
>>> implementor() call. If there's any concurrent class loading happening
>>> which introduces more implementors, corresponding nmethod dependency
>>> will invalidate the nmethod during installation attempt.
>>>
>>> Testing: hs-precheckin-comp, tier1, tier2
>>>
>>> Thanks!
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>>
>>> [1] http://hg.openjdk.java.net/jdk/jdk/file/9c0715c5bbf3/src/hotspot/share/ci/ciInstanceKlass.cpp#l617
>>>
>>> [2] http://hg.openjdk.java.net/jdk/jdk/file/9c0715c5bbf3/src/hotspot/share/ci/ciInstanceKlass.hpp#l72
>>>
>>> [3] https://jbs.oracle.com/browse/JDK-6986483
>>
>