RFR (XXS): 8179244: Assert failed in instanceMirrorKlass.inline.hpp

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR (XXS): 8179244: Assert failed in instanceMirrorKlass.inline.hpp

Thomas Schatzl
Hi all,

  can I have reviews for this small comment update that clarifies that
the assert in that location is too strong and should be commented out.

The issue is that an assert fails during CMS precleaning if class
loading occurred where the mirror ended up in the old generation (in
some situations CMS allocates directly into old gen). This assert found
that a klass that has had a NULL value suddenly changed its value to
something else.

This is normal behavior in the situation described above.

Also checked that we do not miss iterating over a CLD for such oops -
there is actually explicit support in class loading for this, where
such CLDs (and their mirrors) are kept alive until remark and iterated
over again there. (Look for "CMS support" in classLoaderData.?pp).

CR:
https://bugs.openjdk.java.net/browse/JDK-8179244
Webrev:
http://cr.openjdk.java.net/~tschatzl/8179244/webrev/
Testing:
local compilation (just a comment added)

Thanks,
  Thomas

Reply | Threaded
Open this post in threaded view
|

Re: RFR (XXS): 8179244: Assert failed in instanceMirrorKlass.inline.hpp

Erik Helin-2
On 11/20/2017 12:56 PM, Thomas Schatzl wrote:

> Hi all,
>
>    can I have reviews for this small comment update that clarifies that
> the assert in that location is too strong and should be commented out.
>
> The issue is that an assert fails during CMS precleaning if class
> loading occurred where the mirror ended up in the old generation (in
> some situations CMS allocates directly into old gen). This assert found
> that a klass that has had a NULL value suddenly changed its value to
> something else.
>
> This is normal behavior in the situation described above.
>
> Also checked that we do not miss iterating over a CLD for such oops -
> there is actually explicit support in class loading for this, where
> such CLDs (and their mirrors) are kept alive until remark and iterated
> over again there. (Look for "CMS support" in classLoaderData.?pp).
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8179244
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8179244/webrev/

Looks good, Reviewed, I think the comment is on the right level.

Although the comment and the bug (and understanding how this all hangs
together) is far from trivial, I think this actual patch is trivial (it
just adds a comment). Therefore, in my opinion, this patch only requires
one (R)eviewer.

Thanks,
Erik

> Testing:
> local compilation (just a comment added)
>
> Thanks,
>    Thomas
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR (XXS): 8179244: Assert failed in instanceMirrorKlass.inline.hpp

Thomas Schatzl
Hi Erik,

On Wed, 2017-11-22 at 16:49 +0100, Erik Helin wrote:

> On 11/20/2017 12:56 PM, Thomas Schatzl wrote:
> > Hi all,
> >
> >    can I have reviews for this small comment update that clarifies
> > that the assert in that location is too strong and should be
> > commented out.
> >
> > The issue is that an assert fails during CMS precleaning if class
> > loading occurred where the mirror ended up in the old generation
> > (in some situations CMS allocates directly into old gen). This
> > assert found that a klass that has had a NULL value suddenly
> > changed its value to something else.
> >
> > This is normal behavior in the situation described above.
> >
> > Also checked that we do not miss iterating over a CLD for such oops
> > - there is actually explicit support in class loading for this,
> > where such CLDs (and their mirrors) are kept alive until remark and
> > iterated over again there. (Look for "CMS support" in
> > classLoaderData.?pp).
> >
> > CR:
> > https://bugs.openjdk.java.net/browse/JDK-8179244
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8179244/webrev/
>
> Looks good, Reviewed, I think the comment is on the right level.

Thanks.

> Although the comment and the bug (and understanding how this all
> hangs together) is far from trivial, I think this actual patch is
> trivial (it just adds a comment). Therefore, in my opinion, this
> patch only requires one (R)eviewer.

Fine with me. I will push it tomorrow together with the others that are
finished by then.

Thanks,
  Thomas