RFR(M): 8166317: InterpreterCodeSize should be computed

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

RFR(M): 8166317: InterpreterCodeSize should be computed

Volker Simonis
Hi,

can I please have a review and sponsor for the following change:

http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317/
https://bugs.openjdk.java.net/browse/JDK-8166317

The template interpreter is currently created into a compile time
constant, fixed sized part of the CodeCache. This constant (i.e.
'TemplateInterpreter::InterpreterCodeSize') is specified per platform
in templateInterpreterGenerator_<arch>.cpp and may depend on various
other compile time configurations like for example JVMCI. Also, this
constant is quadrupled for debug builds in order to accommodate for
the additional debugging code.

The problem with this approach is that we have to 'guess' a good value
for 'InterpreterCodeSize'. If the value is too big, we will
unnecessarily waste CodeCache because the unused parts of the code
cache allocated for the interpreter won't be returned back to the
CodeCache. If the value is too small the VM may fail to initialize
with "not enough space for interpreter generation". The situation is
further complicated by the fact that some dynamic, run-time
configuration options like debugging/JVMTI, compressed oops, implicit
null checks, etc. may influence the size of the generated interpreter.

Currently, the used/wasted ratio for the interpreter part of the code
cache (which can be dumped with -XX:+PrintInterpreter) looks as
follows for jdk10 on Linux/x86_64:

dbg/JVMTI
-------------
code size        =    475K bytes
total space      =   1071K bytes
wasted space     =    596K bytes

dbg
-------------
code size        =    262K bytes
total space      =   1071K bytes
wasted space     =    809K bytes

opt/JVMTI
-------------
code size        =    195K bytes
total space      =    267K bytes
wasted space     =     72K bytes

opt
-------------
code size        =    124K bytes
total space      =    267K bytes
wasted space     =    143K bytes

Unfortunately it is not easy to compute the size of the generated
interpreter dynamically (we would actually have to generate it two
times in order to do that). It is also not easy to create the
interpreter into a bigger, temporary buffer and move it into an
exactly sized buffer afterwards because the interpreter code is not
relocatable (and the assignment of the various entry points is spread
out over many code locations).

But what we can actually do quite easily is to return the unused part
of the initially allocated memory back to the CodeCache. This is
possible for two reasons. First, the interpreter codelets and stubs
are generated "densely" (see CodeletMark constructor/destructor), i.e.
the unused space of the initially allocated memory is located at the
end of the reserved memory. Second, te interpreter is generated in a
very early stage during VM startup ('interpreter_init()' is called
early from 'init_globals()'). During this early stage we're still
single threaded and can be sure that nobody else is allocating from
the CodeCache while we're generating the interpreter.

So I've introduced a new method 'CodeCache::free_unused_tail(CodeBlob*
cb, size_t used)' which frees the unused tail of the interpreter
CodeBlob. It has a guarantee which makes sure that is only called for
the interpreter CodeBlob. 'free_unused_tail()' calls
'CodeHeap::deallocate_tail(void* p, size_t used_size)' on the
corresponding CodeHeap which in turn checks (with another guarantee)
that there have been no intermediate allocations and returns the
unused tail of the corresponding HeapBlock back to the CodeHeap.

With this change, theres no more waste in the CodeCache after
interpreter generation and the output of -XX:+PrintInterpreter looks
as follows:

dbg/JVMTI
-------------
code size        =    475K bytes
total space      =    475K bytes
wasted space     =      0K bytes

dbg
-------------
code size        =    262K bytes
total space      =    262K bytes
wasted space     =      0K bytes

opt/JVMTI
-------------
code size        =    195K bytes
total space      =    195K bytes
wasted space     =      0K bytes

opt
-------------
code size        =    124K bytes
total space      =    124K bytes
wasted space     =      0K bytes

So in the normal case (product build without debugging) we're saving
143K of CodeCache. While this is not overly impressing I think the
major benefit of this change is that we can increase the default value
for 'InterpreterCodeSize' in the future without much reasoning (e.g.
doubling it if it is too small) because the unused part will be
returned back to the CodeCache.

I've successfully (with the exception described below) tested the
change by running the hotspot JTreg tests with and without
SegmentedCodeCache.

While working on this, I found another problem which is related to the
fix of JDK-8183573 and leads to crashes when executing the JTreg test
compiler/codecache/stress/ReturnBlobToWrongHeapTest.java.

The problem is that JDK-8183573 replaced

  virtual bool contains_blob(const CodeBlob* blob) const { return
low_boundary() <= (char*) blob && (char*) blob < high(); }

by:

  bool contains_blob(const CodeBlob* blob) const { return
contains(blob->code_begin()); }

But that my be wrong in the corner case where the size of the
CodeBlob's payload is zero (i.e. the CodeBlob consists only of the
'header' - i.e. the C++ object itself) because in that case
CodeBlob::code_begin() points right behind the CodeBlob's header which
is a memory location which doesn't belong to the CodeBlob anymore.

This exact corner case is exercised by ReturnBlobToWrongHeapTest which
allocates CodeBlobs of size zero (i.e. zero 'payload') with the help
of sun.hotspot.WhiteBox.allocateCodeBlob() until the CodeCache fills
up. The test first fills the 'non-profiled nmethods' CodeHeap. If the
'non-profiled nmethods' CodeHeap is full, the VM automatically tries
to allocate from the 'profiled nmethods' CodeHeap until that fills up
as well.  But in the CodeCache the 'profiled nmethods' CodeHeap is
located right before the non-profiled nmethods' CodeHeap. So if the
last CodeBlob allocated from the 'profiled nmethods' CodeHeap has a
payload size of zero and uses all the CodeHeaps remaining size, we
will end up with a CodeBlob whose code_begin() address will point
right behind the actual CodeHeap (i.e. it will point right at the
beginning of the adjacent, 'non-profiled nmethods' CodeHeap). This
will result in the following guarantee to fire, when we try to free
the last allocated CodeBlob (i.e. the one allocated at the end of the
'profiled nmethods' CodeHeap which has its code_begin() address
pointing at the the beginning of the adjacent, 'non-profiled nmethods'
CodeHeap) with sun.hotspot.WhiteBox.freeCodeBlob():

# A fatal error has been detected by the Java Runtime Environment:
#
#  Internal Error (heap.cpp:248), pid=27586, tid=27587
#  guarantee((char*) b >= _memory.low_boundary() && (char*) b <
_memory.high()) failed: The block to be deallocated 0x00007fffe6666f80
is not within the heap starting with 0x00007fffe6667000 and ending
with 0x00007fffe6ba000

The fix is trivial so I'll include it into this change: just revert
the part of JDK-8183573 mentioned before such that contains_blob()
again uses the address of the CodeBlob instead of
CodeBlob->code_begin().

Thank you and best regards,
Volker
Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8166317: InterpreterCodeSize should be computed

Claes Redestad


On 2017-08-31 08:54, Volker Simonis wrote:

> While working on this, I found another problem which is related to the
> fix of JDK-8183573 and leads to crashes when executing the JTreg test
> compiler/codecache/stress/ReturnBlobToWrongHeapTest.java.
>
> The problem is that JDK-8183573 replaced
>
>    virtual bool contains_blob(const CodeBlob* blob) const { return
> low_boundary() <= (char*) blob && (char*) blob < high(); }
>
> by:
>
>    bool contains_blob(const CodeBlob* blob) const { return
> contains(blob->code_begin()); }
>
> But that my be wrong in the corner case where the size of the
> CodeBlob's payload is zero (i.e. the CodeBlob consists only of the
> 'header' - i.e. the C++ object itself) because in that case
> CodeBlob::code_begin() points right behind the CodeBlob's header which
> is a memory location which doesn't belong to the CodeBlob anymore.

I recall this change was somehow necessary to allow merging
AOTCodeHeap::contains_blob and CodeHead::contains_blob into
one devirtualized method, so you need to ensure all AOT tests
pass with this change (on linux-x64).

I can't help to wonder if we'd not be better served by disallowing
zero-sized payloads. Is this something that can ever actually
happen except by abuse of the white box API?

/Claes
Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8166317: InterpreterCodeSize should be computed

coleen.phillimore
In reply to this post by Volker Simonis

Hi Volker,

Thank you for doing this.  I will review this and sponsor it.
Unfortunately the jdk10/hs repository is going to be closed for a few
weeks while the consolidation effort is taking place, but that should
give plenty of time for reviews.

thanks,
Coleen

On 8/31/17 2:54 AM, Volker Simonis wrote:

> Hi,
>
> can I please have a review and sponsor for the following change:
>
> http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317/
> https://bugs.openjdk.java.net/browse/JDK-8166317
>
> The template interpreter is currently created into a compile time
> constant, fixed sized part of the CodeCache. This constant (i.e.
> 'TemplateInterpreter::InterpreterCodeSize') is specified per platform
> in templateInterpreterGenerator_<arch>.cpp and may depend on various
> other compile time configurations like for example JVMCI. Also, this
> constant is quadrupled for debug builds in order to accommodate for
> the additional debugging code.
>
> The problem with this approach is that we have to 'guess' a good value
> for 'InterpreterCodeSize'. If the value is too big, we will
> unnecessarily waste CodeCache because the unused parts of the code
> cache allocated for the interpreter won't be returned back to the
> CodeCache. If the value is too small the VM may fail to initialize
> with "not enough space for interpreter generation". The situation is
> further complicated by the fact that some dynamic, run-time
> configuration options like debugging/JVMTI, compressed oops, implicit
> null checks, etc. may influence the size of the generated interpreter.
>
> Currently, the used/wasted ratio for the interpreter part of the code
> cache (which can be dumped with -XX:+PrintInterpreter) looks as
> follows for jdk10 on Linux/x86_64:
>
> dbg/JVMTI
> -------------
> code size        =    475K bytes
> total space      =   1071K bytes
> wasted space     =    596K bytes
>
> dbg
> -------------
> code size        =    262K bytes
> total space      =   1071K bytes
> wasted space     =    809K bytes
>
> opt/JVMTI
> -------------
> code size        =    195K bytes
> total space      =    267K bytes
> wasted space     =     72K bytes
>
> opt
> -------------
> code size        =    124K bytes
> total space      =    267K bytes
> wasted space     =    143K bytes
>
> Unfortunately it is not easy to compute the size of the generated
> interpreter dynamically (we would actually have to generate it two
> times in order to do that). It is also not easy to create the
> interpreter into a bigger, temporary buffer and move it into an
> exactly sized buffer afterwards because the interpreter code is not
> relocatable (and the assignment of the various entry points is spread
> out over many code locations).
>
> But what we can actually do quite easily is to return the unused part
> of the initially allocated memory back to the CodeCache. This is
> possible for two reasons. First, the interpreter codelets and stubs
> are generated "densely" (see CodeletMark constructor/destructor), i.e.
> the unused space of the initially allocated memory is located at the
> end of the reserved memory. Second, te interpreter is generated in a
> very early stage during VM startup ('interpreter_init()' is called
> early from 'init_globals()'). During this early stage we're still
> single threaded and can be sure that nobody else is allocating from
> the CodeCache while we're generating the interpreter.
>
> So I've introduced a new method 'CodeCache::free_unused_tail(CodeBlob*
> cb, size_t used)' which frees the unused tail of the interpreter
> CodeBlob. It has a guarantee which makes sure that is only called for
> the interpreter CodeBlob. 'free_unused_tail()' calls
> 'CodeHeap::deallocate_tail(void* p, size_t used_size)' on the
> corresponding CodeHeap which in turn checks (with another guarantee)
> that there have been no intermediate allocations and returns the
> unused tail of the corresponding HeapBlock back to the CodeHeap.
>
> With this change, theres no more waste in the CodeCache after
> interpreter generation and the output of -XX:+PrintInterpreter looks
> as follows:
>
> dbg/JVMTI
> -------------
> code size        =    475K bytes
> total space      =    475K bytes
> wasted space     =      0K bytes
>
> dbg
> -------------
> code size        =    262K bytes
> total space      =    262K bytes
> wasted space     =      0K bytes
>
> opt/JVMTI
> -------------
> code size        =    195K bytes
> total space      =    195K bytes
> wasted space     =      0K bytes
>
> opt
> -------------
> code size        =    124K bytes
> total space      =    124K bytes
> wasted space     =      0K bytes
>
> So in the normal case (product build without debugging) we're saving
> 143K of CodeCache. While this is not overly impressing I think the
> major benefit of this change is that we can increase the default value
> for 'InterpreterCodeSize' in the future without much reasoning (e.g.
> doubling it if it is too small) because the unused part will be
> returned back to the CodeCache.
>
> I've successfully (with the exception described below) tested the
> change by running the hotspot JTreg tests with and without
> SegmentedCodeCache.
>
> While working on this, I found another problem which is related to the
> fix of JDK-8183573 and leads to crashes when executing the JTreg test
> compiler/codecache/stress/ReturnBlobToWrongHeapTest.java.
>
> The problem is that JDK-8183573 replaced
>
>    virtual bool contains_blob(const CodeBlob* blob) const { return
> low_boundary() <= (char*) blob && (char*) blob < high(); }
>
> by:
>
>    bool contains_blob(const CodeBlob* blob) const { return
> contains(blob->code_begin()); }
>
> But that my be wrong in the corner case where the size of the
> CodeBlob's payload is zero (i.e. the CodeBlob consists only of the
> 'header' - i.e. the C++ object itself) because in that case
> CodeBlob::code_begin() points right behind the CodeBlob's header which
> is a memory location which doesn't belong to the CodeBlob anymore.
>
> This exact corner case is exercised by ReturnBlobToWrongHeapTest which
> allocates CodeBlobs of size zero (i.e. zero 'payload') with the help
> of sun.hotspot.WhiteBox.allocateCodeBlob() until the CodeCache fills
> up. The test first fills the 'non-profiled nmethods' CodeHeap. If the
> 'non-profiled nmethods' CodeHeap is full, the VM automatically tries
> to allocate from the 'profiled nmethods' CodeHeap until that fills up
> as well.  But in the CodeCache the 'profiled nmethods' CodeHeap is
> located right before the non-profiled nmethods' CodeHeap. So if the
> last CodeBlob allocated from the 'profiled nmethods' CodeHeap has a
> payload size of zero and uses all the CodeHeaps remaining size, we
> will end up with a CodeBlob whose code_begin() address will point
> right behind the actual CodeHeap (i.e. it will point right at the
> beginning of the adjacent, 'non-profiled nmethods' CodeHeap). This
> will result in the following guarantee to fire, when we try to free
> the last allocated CodeBlob (i.e. the one allocated at the end of the
> 'profiled nmethods' CodeHeap which has its code_begin() address
> pointing at the the beginning of the adjacent, 'non-profiled nmethods'
> CodeHeap) with sun.hotspot.WhiteBox.freeCodeBlob():
>
> # A fatal error has been detected by the Java Runtime Environment:
> #
> #  Internal Error (heap.cpp:248), pid=27586, tid=27587
> #  guarantee((char*) b >= _memory.low_boundary() && (char*) b <
> _memory.high()) failed: The block to be deallocated 0x00007fffe6666f80
> is not within the heap starting with 0x00007fffe6667000 and ending
> with 0x00007fffe6ba000
>
> The fix is trivial so I'll include it into this change: just revert
> the part of JDK-8183573 mentioned before such that contains_blob()
> again uses the address of the CodeBlob instead of
> CodeBlob->code_begin().
>
> Thank you and best regards,
> Volker

Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8166317: InterpreterCodeSize should be computed

Volker Simonis
In reply to this post by Claes Redestad
On Thu, Aug 31, 2017 at 12:14 PM, Claes Redestad
<[hidden email]> wrote:

>
>
> On 2017-08-31 08:54, Volker Simonis wrote:
>>
>> While working on this, I found another problem which is related to the
>> fix of JDK-8183573 and leads to crashes when executing the JTreg test
>> compiler/codecache/stress/ReturnBlobToWrongHeapTest.java.
>>
>> The problem is that JDK-8183573 replaced
>>
>>    virtual bool contains_blob(const CodeBlob* blob) const { return
>> low_boundary() <= (char*) blob && (char*) blob < high(); }
>>
>> by:
>>
>>    bool contains_blob(const CodeBlob* blob) const { return
>> contains(blob->code_begin()); }
>>
>> But that my be wrong in the corner case where the size of the
>> CodeBlob's payload is zero (i.e. the CodeBlob consists only of the
>> 'header' - i.e. the C++ object itself) because in that case
>> CodeBlob::code_begin() points right behind the CodeBlob's header which
>> is a memory location which doesn't belong to the CodeBlob anymore.
>
>
> I recall this change was somehow necessary to allow merging
> AOTCodeHeap::contains_blob and CodeHead::contains_blob into
> one devirtualized method, so you need to ensure all AOT tests
> pass with this change (on linux-x64).
>

All of hotspot/test/aot and hotspot/test/jvmci executed and passed
successful. Are there any other tests I should check?

That said, it is a little hard to follow the stages of your change. It
seems like http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.00/
was reviewed [1] but then finally the slightly changed version from
http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.01/ was
checked in and linked to the bug report.

The first, reviewed version of the change still had a correct version
of 'CodeHeap::contains_blob(const CodeBlob* blob)' while the second,
checked in version has the faulty version of that method.

I don't know why you finally did that change to 'contains_blob()' but
I don't see any reason why we shouldn't be able to directly use the
blob's address for inclusion checking. From what I understand, it
should ALWAYS be contained in the corresponding CodeHeap so no reason
to mess with 'CodeBlob::code_begin()'.

Please let me know if I'm missing something.

[1] http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2017-July/026624.html

> I can't help to wonder if we'd not be better served by disallowing
> zero-sized payloads. Is this something that can ever actually
> happen except by abuse of the white box API?
>

The corresponding test (ReturnBlobToWrongHeapTest.java) specifically
wants to allocate "segment sized" blocks which is most easily achieved
by allocation zero-sized CodeBlobs. And I think there's nothing wrong
about it if we handle the inclusion tests correctly.

Thank you and best regards,
Volker

> /Claes
Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8166317: InterpreterCodeSize should be computed

Vladimir Kozlov
Very good change. Thank you, Volker.

About contains_blob(). The problem is that AOTCompiledMethod allocated in CHeap and not in aot code section (which is RO):

http://hg.openjdk.java.net/jdk10/hs/hotspot/file/8acd232fb52a/src/share/vm/aot/aotCompiledMethod.hpp#l124

It is allocated in CHeap after AOT library is loaded. Its code_begin() points to AOT code section but AOTCompiledMethod*
points outside it (to normal malloced space) so you can't use (char*)blob address.

There are 2 ways to fix it, I think.
One is to add new field to CodeBlobLayout and set it to blob* address for normal CodeCache blobs and to code_begin for
AOT code.
Second is to use contains(blob->code_end() - 1) assuming that AOT code is never zero.

Thanks,
Vladimir

On 8/31/17 5:43 AM, Volker Simonis wrote:

> On Thu, Aug 31, 2017 at 12:14 PM, Claes Redestad
> <[hidden email]> wrote:
>>
>>
>> On 2017-08-31 08:54, Volker Simonis wrote:
>>>
>>> While working on this, I found another problem which is related to the
>>> fix of JDK-8183573 and leads to crashes when executing the JTreg test
>>> compiler/codecache/stress/ReturnBlobToWrongHeapTest.java.
>>>
>>> The problem is that JDK-8183573 replaced
>>>
>>>     virtual bool contains_blob(const CodeBlob* blob) const { return
>>> low_boundary() <= (char*) blob && (char*) blob < high(); }
>>>
>>> by:
>>>
>>>     bool contains_blob(const CodeBlob* blob) const { return
>>> contains(blob->code_begin()); }
>>>
>>> But that my be wrong in the corner case where the size of the
>>> CodeBlob's payload is zero (i.e. the CodeBlob consists only of the
>>> 'header' - i.e. the C++ object itself) because in that case
>>> CodeBlob::code_begin() points right behind the CodeBlob's header which
>>> is a memory location which doesn't belong to the CodeBlob anymore.
>>
>>
>> I recall this change was somehow necessary to allow merging
>> AOTCodeHeap::contains_blob and CodeHead::contains_blob into
>> one devirtualized method, so you need to ensure all AOT tests
>> pass with this change (on linux-x64).
>>
>
> All of hotspot/test/aot and hotspot/test/jvmci executed and passed
> successful. Are there any other tests I should check?
>
> That said, it is a little hard to follow the stages of your change. It
> seems like http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.00/
> was reviewed [1] but then finally the slightly changed version from
> http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.01/ was
> checked in and linked to the bug report.
>
> The first, reviewed version of the change still had a correct version
> of 'CodeHeap::contains_blob(const CodeBlob* blob)' while the second,
> checked in version has the faulty version of that method.
>
> I don't know why you finally did that change to 'contains_blob()' but
> I don't see any reason why we shouldn't be able to directly use the
> blob's address for inclusion checking. From what I understand, it
> should ALWAYS be contained in the corresponding CodeHeap so no reason
> to mess with 'CodeBlob::code_begin()'.
>
> Please let me know if I'm missing something.
>
> [1] http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2017-July/026624.html
>
>> I can't help to wonder if we'd not be better served by disallowing
>> zero-sized payloads. Is this something that can ever actually
>> happen except by abuse of the white box API?
>>
>
> The corresponding test (ReturnBlobToWrongHeapTest.java) specifically
> wants to allocate "segment sized" blocks which is most easily achieved
> by allocation zero-sized CodeBlobs. And I think there's nothing wrong
> about it if we handle the inclusion tests correctly.
>
> Thank you and best regards,
> Volker
>
>> /Claes
Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8166317: InterpreterCodeSize should be computed

Volker Simonis
On Thu, Aug 31, 2017 at 6:05 PM, Vladimir Kozlov
<[hidden email]> wrote:

> Very good change. Thank you, Volker.
>
> About contains_blob(). The problem is that AOTCompiledMethod allocated in
> CHeap and not in aot code section (which is RO):
>
> http://hg.openjdk.java.net/jdk10/hs/hotspot/file/8acd232fb52a/src/share/vm/aot/aotCompiledMethod.hpp#l124
>
> It is allocated in CHeap after AOT library is loaded. Its code_begin()
> points to AOT code section but AOTCompiledMethod* points outside it (to
> normal malloced space) so you can't use (char*)blob address.
>

Thanks for the explanation - now I got it.

> There are 2 ways to fix it, I think.
> One is to add new field to CodeBlobLayout and set it to blob* address for
> normal CodeCache blobs and to code_begin for AOT code.
> Second is to use contains(blob->code_end() - 1) assuming that AOT code is
> never zero.
>

I'll give it a try tomorrow and will send out a new webrev.

Regards,
Volker

> Thanks,
> Vladimir
>
>
> On 8/31/17 5:43 AM, Volker Simonis wrote:
>>
>> On Thu, Aug 31, 2017 at 12:14 PM, Claes Redestad
>> <[hidden email]> wrote:
>>>
>>>
>>>
>>> On 2017-08-31 08:54, Volker Simonis wrote:
>>>>
>>>>
>>>> While working on this, I found another problem which is related to the
>>>> fix of JDK-8183573 and leads to crashes when executing the JTreg test
>>>> compiler/codecache/stress/ReturnBlobToWrongHeapTest.java.
>>>>
>>>> The problem is that JDK-8183573 replaced
>>>>
>>>>     virtual bool contains_blob(const CodeBlob* blob) const { return
>>>> low_boundary() <= (char*) blob && (char*) blob < high(); }
>>>>
>>>> by:
>>>>
>>>>     bool contains_blob(const CodeBlob* blob) const { return
>>>> contains(blob->code_begin()); }
>>>>
>>>> But that my be wrong in the corner case where the size of the
>>>> CodeBlob's payload is zero (i.e. the CodeBlob consists only of the
>>>> 'header' - i.e. the C++ object itself) because in that case
>>>> CodeBlob::code_begin() points right behind the CodeBlob's header which
>>>> is a memory location which doesn't belong to the CodeBlob anymore.
>>>
>>>
>>>
>>> I recall this change was somehow necessary to allow merging
>>> AOTCodeHeap::contains_blob and CodeHead::contains_blob into
>>> one devirtualized method, so you need to ensure all AOT tests
>>> pass with this change (on linux-x64).
>>>
>>
>> All of hotspot/test/aot and hotspot/test/jvmci executed and passed
>> successful. Are there any other tests I should check?
>>
>> That said, it is a little hard to follow the stages of your change. It
>> seems like
>> http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.00/
>> was reviewed [1] but then finally the slightly changed version from
>> http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.01/ was
>> checked in and linked to the bug report.
>>
>> The first, reviewed version of the change still had a correct version
>> of 'CodeHeap::contains_blob(const CodeBlob* blob)' while the second,
>> checked in version has the faulty version of that method.
>>
>> I don't know why you finally did that change to 'contains_blob()' but
>> I don't see any reason why we shouldn't be able to directly use the
>> blob's address for inclusion checking. From what I understand, it
>> should ALWAYS be contained in the corresponding CodeHeap so no reason
>> to mess with 'CodeBlob::code_begin()'.
>>
>> Please let me know if I'm missing something.
>>
>> [1]
>> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2017-July/026624.html
>>
>>> I can't help to wonder if we'd not be better served by disallowing
>>> zero-sized payloads. Is this something that can ever actually
>>> happen except by abuse of the white box API?
>>>
>>
>> The corresponding test (ReturnBlobToWrongHeapTest.java) specifically
>> wants to allocate "segment sized" blocks which is most easily achieved
>> by allocation zero-sized CodeBlobs. And I think there's nothing wrong
>> about it if we handle the inclusion tests correctly.
>>
>> Thank you and best regards,
>> Volker
>>
>>> /Claes
Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8166317: InterpreterCodeSize should be computed

Volker Simonis
Hi,

I've decided to split the fix for the 'CodeHeap::contains_blob()'
problem into its own issue "8187091: ReturnBlobToWrongHeapTest fails
because of problems in CodeHeap::contains_blob()"
(https://bugs.openjdk.java.net/browse/JDK-8187091) and started a new
review thread for discussing it at:
http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-September/028206.html

So please lets keep this thread for discussing the interpreter code
size issue only. I've prepared a new version of the webrev which is
the same as the first one with the only difference that the change to
'CodeHeap::contains_blob()' has been removed:

http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317.v1/

Thanks,
Volker


On Thu, Aug 31, 2017 at 6:35 PM, Volker Simonis
<[hidden email]> wrote:

> On Thu, Aug 31, 2017 at 6:05 PM, Vladimir Kozlov
> <[hidden email]> wrote:
>> Very good change. Thank you, Volker.
>>
>> About contains_blob(). The problem is that AOTCompiledMethod allocated in
>> CHeap and not in aot code section (which is RO):
>>
>> http://hg.openjdk.java.net/jdk10/hs/hotspot/file/8acd232fb52a/src/share/vm/aot/aotCompiledMethod.hpp#l124
>>
>> It is allocated in CHeap after AOT library is loaded. Its code_begin()
>> points to AOT code section but AOTCompiledMethod* points outside it (to
>> normal malloced space) so you can't use (char*)blob address.
>>
>
> Thanks for the explanation - now I got it.
>
>> There are 2 ways to fix it, I think.
>> One is to add new field to CodeBlobLayout and set it to blob* address for
>> normal CodeCache blobs and to code_begin for AOT code.
>> Second is to use contains(blob->code_end() - 1) assuming that AOT code is
>> never zero.
>>
>
> I'll give it a try tomorrow and will send out a new webrev.
>
> Regards,
> Volker
>
>> Thanks,
>> Vladimir
>>
>>
>> On 8/31/17 5:43 AM, Volker Simonis wrote:
>>>
>>> On Thu, Aug 31, 2017 at 12:14 PM, Claes Redestad
>>> <[hidden email]> wrote:
>>>>
>>>>
>>>>
>>>> On 2017-08-31 08:54, Volker Simonis wrote:
>>>>>
>>>>>
>>>>> While working on this, I found another problem which is related to the
>>>>> fix of JDK-8183573 and leads to crashes when executing the JTreg test
>>>>> compiler/codecache/stress/ReturnBlobToWrongHeapTest.java.
>>>>>
>>>>> The problem is that JDK-8183573 replaced
>>>>>
>>>>>     virtual bool contains_blob(const CodeBlob* blob) const { return
>>>>> low_boundary() <= (char*) blob && (char*) blob < high(); }
>>>>>
>>>>> by:
>>>>>
>>>>>     bool contains_blob(const CodeBlob* blob) const { return
>>>>> contains(blob->code_begin()); }
>>>>>
>>>>> But that my be wrong in the corner case where the size of the
>>>>> CodeBlob's payload is zero (i.e. the CodeBlob consists only of the
>>>>> 'header' - i.e. the C++ object itself) because in that case
>>>>> CodeBlob::code_begin() points right behind the CodeBlob's header which
>>>>> is a memory location which doesn't belong to the CodeBlob anymore.
>>>>
>>>>
>>>>
>>>> I recall this change was somehow necessary to allow merging
>>>> AOTCodeHeap::contains_blob and CodeHead::contains_blob into
>>>> one devirtualized method, so you need to ensure all AOT tests
>>>> pass with this change (on linux-x64).
>>>>
>>>
>>> All of hotspot/test/aot and hotspot/test/jvmci executed and passed
>>> successful. Are there any other tests I should check?
>>>
>>> That said, it is a little hard to follow the stages of your change. It
>>> seems like
>>> http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.00/
>>> was reviewed [1] but then finally the slightly changed version from
>>> http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.01/ was
>>> checked in and linked to the bug report.
>>>
>>> The first, reviewed version of the change still had a correct version
>>> of 'CodeHeap::contains_blob(const CodeBlob* blob)' while the second,
>>> checked in version has the faulty version of that method.
>>>
>>> I don't know why you finally did that change to 'contains_blob()' but
>>> I don't see any reason why we shouldn't be able to directly use the
>>> blob's address for inclusion checking. From what I understand, it
>>> should ALWAYS be contained in the corresponding CodeHeap so no reason
>>> to mess with 'CodeBlob::code_begin()'.
>>>
>>> Please let me know if I'm missing something.
>>>
>>> [1]
>>> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2017-July/026624.html
>>>
>>>> I can't help to wonder if we'd not be better served by disallowing
>>>> zero-sized payloads. Is this something that can ever actually
>>>> happen except by abuse of the white box API?
>>>>
>>>
>>> The corresponding test (ReturnBlobToWrongHeapTest.java) specifically
>>> wants to allocate "segment sized" blocks which is most easily achieved
>>> by allocation zero-sized CodeBlobs. And I think there's nothing wrong
>>> about it if we handle the inclusion tests correctly.
>>>
>>> Thank you and best regards,
>>> Volker
>>>
>>>> /Claes
Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8166317: InterpreterCodeSize should be computed

Vladimir Kozlov
May be add new CodeBlob's method to adjust sizes instead of directly setting them in  CodeCache::free_unused_tail().
Then you would not need friend class CodeCache in CodeBlob.

Also I think adjustment to header_size should be done in CodeCache::free_unused_tail() to limit scope of code who knows
about blob layout.

Thanks,
Vladimir

On 9/1/17 8:46 AM, Volker Simonis wrote:

> Hi,
>
> I've decided to split the fix for the 'CodeHeap::contains_blob()'
> problem into its own issue "8187091: ReturnBlobToWrongHeapTest fails
> because of problems in CodeHeap::contains_blob()"
> (https://bugs.openjdk.java.net/browse/JDK-8187091) and started a new
> review thread for discussing it at:
> http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-September/028206.html
>
> So please lets keep this thread for discussing the interpreter code
> size issue only. I've prepared a new version of the webrev which is
> the same as the first one with the only difference that the change to
> 'CodeHeap::contains_blob()' has been removed:
>
> http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317.v1/
>
> Thanks,
> Volker
>
>
> On Thu, Aug 31, 2017 at 6:35 PM, Volker Simonis
> <[hidden email]> wrote:
>> On Thu, Aug 31, 2017 at 6:05 PM, Vladimir Kozlov
>> <[hidden email]> wrote:
>>> Very good change. Thank you, Volker.
>>>
>>> About contains_blob(). The problem is that AOTCompiledMethod allocated in
>>> CHeap and not in aot code section (which is RO):
>>>
>>> http://hg.openjdk.java.net/jdk10/hs/hotspot/file/8acd232fb52a/src/share/vm/aot/aotCompiledMethod.hpp#l124
>>>
>>> It is allocated in CHeap after AOT library is loaded. Its code_begin()
>>> points to AOT code section but AOTCompiledMethod* points outside it (to
>>> normal malloced space) so you can't use (char*)blob address.
>>>
>>
>> Thanks for the explanation - now I got it.
>>
>>> There are 2 ways to fix it, I think.
>>> One is to add new field to CodeBlobLayout and set it to blob* address for
>>> normal CodeCache blobs and to code_begin for AOT code.
>>> Second is to use contains(blob->code_end() - 1) assuming that AOT code is
>>> never zero.
>>>
>>
>> I'll give it a try tomorrow and will send out a new webrev.
>>
>> Regards,
>> Volker
>>
>>> Thanks,
>>> Vladimir
>>>
>>>
>>> On 8/31/17 5:43 AM, Volker Simonis wrote:
>>>>
>>>> On Thu, Aug 31, 2017 at 12:14 PM, Claes Redestad
>>>> <[hidden email]> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 2017-08-31 08:54, Volker Simonis wrote:
>>>>>>
>>>>>>
>>>>>> While working on this, I found another problem which is related to the
>>>>>> fix of JDK-8183573 and leads to crashes when executing the JTreg test
>>>>>> compiler/codecache/stress/ReturnBlobToWrongHeapTest.java.
>>>>>>
>>>>>> The problem is that JDK-8183573 replaced
>>>>>>
>>>>>>      virtual bool contains_blob(const CodeBlob* blob) const { return
>>>>>> low_boundary() <= (char*) blob && (char*) blob < high(); }
>>>>>>
>>>>>> by:
>>>>>>
>>>>>>      bool contains_blob(const CodeBlob* blob) const { return
>>>>>> contains(blob->code_begin()); }
>>>>>>
>>>>>> But that my be wrong in the corner case where the size of the
>>>>>> CodeBlob's payload is zero (i.e. the CodeBlob consists only of the
>>>>>> 'header' - i.e. the C++ object itself) because in that case
>>>>>> CodeBlob::code_begin() points right behind the CodeBlob's header which
>>>>>> is a memory location which doesn't belong to the CodeBlob anymore.
>>>>>
>>>>>
>>>>>
>>>>> I recall this change was somehow necessary to allow merging
>>>>> AOTCodeHeap::contains_blob and CodeHead::contains_blob into
>>>>> one devirtualized method, so you need to ensure all AOT tests
>>>>> pass with this change (on linux-x64).
>>>>>
>>>>
>>>> All of hotspot/test/aot and hotspot/test/jvmci executed and passed
>>>> successful. Are there any other tests I should check?
>>>>
>>>> That said, it is a little hard to follow the stages of your change. It
>>>> seems like
>>>> http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.00/
>>>> was reviewed [1] but then finally the slightly changed version from
>>>> http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.01/ was
>>>> checked in and linked to the bug report.
>>>>
>>>> The first, reviewed version of the change still had a correct version
>>>> of 'CodeHeap::contains_blob(const CodeBlob* blob)' while the second,
>>>> checked in version has the faulty version of that method.
>>>>
>>>> I don't know why you finally did that change to 'contains_blob()' but
>>>> I don't see any reason why we shouldn't be able to directly use the
>>>> blob's address for inclusion checking. From what I understand, it
>>>> should ALWAYS be contained in the corresponding CodeHeap so no reason
>>>> to mess with 'CodeBlob::code_begin()'.
>>>>
>>>> Please let me know if I'm missing something.
>>>>
>>>> [1]
>>>> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2017-July/026624.html
>>>>
>>>>> I can't help to wonder if we'd not be better served by disallowing
>>>>> zero-sized payloads. Is this something that can ever actually
>>>>> happen except by abuse of the white box API?
>>>>>
>>>>
>>>> The corresponding test (ReturnBlobToWrongHeapTest.java) specifically
>>>> wants to allocate "segment sized" blocks which is most easily achieved
>>>> by allocation zero-sized CodeBlobs. And I think there's nothing wrong
>>>> about it if we handle the inclusion tests correctly.
>>>>
>>>> Thank you and best regards,
>>>> Volker
>>>>
>>>>> /Claes
Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8166317: InterpreterCodeSize should be computed

Volker Simonis
On Fri, Sep 1, 2017 at 6:16 PM, Vladimir Kozlov
<[hidden email]> wrote:
> May be add new CodeBlob's method to adjust sizes instead of directly setting
> them in  CodeCache::free_unused_tail(). Then you would not need friend class
> CodeCache in CodeBlob.
>

Changed as suggested (I didn't liked the friend declaration as well :)

> Also I think adjustment to header_size should be done in
> CodeCache::free_unused_tail() to limit scope of code who knows about blob
> layout.
>

Yes, that's much cleaner. Please find the updated webrev here:

http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317.v2/

I've also found another "day 1" problem in StubQueue::next():

   Stub* next(Stub* s) const                      { int i =
index_of(s) + stub_size(s);
-                                                   if (i ==
_buffer_limit) i = 0;
+                                                   // Only wrap
around in the non-contiguous case (see stubss.cpp)
+                                                   if (i ==
_buffer_limit && _queue_end < _buffer_limit) i = 0;
                                                    return (i ==
_queue_end) ? NULL : stub_at(i);
                                                  }

The problem was that the method was not prepared to handle the case
where _buffer_limit == _queue_end == _buffer_size which lead to an
infinite recursion when iterating over a StubQueue with
StubQueue::next() until next() returns NULL (as this was for example
done with -XX:+PrintInterpreter). But with the new, trimmed CodeBlob
we run into exactly this situation.

While doing this last fix I also noticed that "StubQueue::stubs_do()",
"StubQueue::queues_do()" and "StubQueue::register_queue()" don't seem
to be used anywhere in the open code base (please correct me if I'm
wrong). What do you think, maybe we should remove this code in a
follow up change if it is really not needed?

Finally, could you please run the new version through JPRT and sponsor
it once jdk10/hs will be opened again?

Thanks,
Volker

> Thanks,
> Vladimir
>
>
> On 9/1/17 8:46 AM, Volker Simonis wrote:
>>
>> Hi,
>>
>> I've decided to split the fix for the 'CodeHeap::contains_blob()'
>> problem into its own issue "8187091: ReturnBlobToWrongHeapTest fails
>> because of problems in CodeHeap::contains_blob()"
>> (https://bugs.openjdk.java.net/browse/JDK-8187091) and started a new
>> review thread for discussing it at:
>>
>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-September/028206.html
>>
>> So please lets keep this thread for discussing the interpreter code
>> size issue only. I've prepared a new version of the webrev which is
>> the same as the first one with the only difference that the change to
>> 'CodeHeap::contains_blob()' has been removed:
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317.v1/
>>
>> Thanks,
>> Volker
>>
>>
>> On Thu, Aug 31, 2017 at 6:35 PM, Volker Simonis
>> <[hidden email]> wrote:
>>>
>>> On Thu, Aug 31, 2017 at 6:05 PM, Vladimir Kozlov
>>> <[hidden email]> wrote:
>>>>
>>>> Very good change. Thank you, Volker.
>>>>
>>>> About contains_blob(). The problem is that AOTCompiledMethod allocated
>>>> in
>>>> CHeap and not in aot code section (which is RO):
>>>>
>>>>
>>>> http://hg.openjdk.java.net/jdk10/hs/hotspot/file/8acd232fb52a/src/share/vm/aot/aotCompiledMethod.hpp#l124
>>>>
>>>> It is allocated in CHeap after AOT library is loaded. Its code_begin()
>>>> points to AOT code section but AOTCompiledMethod* points outside it (to
>>>> normal malloced space) so you can't use (char*)blob address.
>>>>
>>>
>>> Thanks for the explanation - now I got it.
>>>
>>>> There are 2 ways to fix it, I think.
>>>> One is to add new field to CodeBlobLayout and set it to blob* address
>>>> for
>>>> normal CodeCache blobs and to code_begin for AOT code.
>>>> Second is to use contains(blob->code_end() - 1) assuming that AOT code
>>>> is
>>>> never zero.
>>>>
>>>
>>> I'll give it a try tomorrow and will send out a new webrev.
>>>
>>> Regards,
>>> Volker
>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>>
>>>> On 8/31/17 5:43 AM, Volker Simonis wrote:
>>>>>
>>>>>
>>>>> On Thu, Aug 31, 2017 at 12:14 PM, Claes Redestad
>>>>> <[hidden email]> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 2017-08-31 08:54, Volker Simonis wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> While working on this, I found another problem which is related to
>>>>>>> the
>>>>>>> fix of JDK-8183573 and leads to crashes when executing the JTreg test
>>>>>>> compiler/codecache/stress/ReturnBlobToWrongHeapTest.java.
>>>>>>>
>>>>>>> The problem is that JDK-8183573 replaced
>>>>>>>
>>>>>>>      virtual bool contains_blob(const CodeBlob* blob) const { return
>>>>>>> low_boundary() <= (char*) blob && (char*) blob < high(); }
>>>>>>>
>>>>>>> by:
>>>>>>>
>>>>>>>      bool contains_blob(const CodeBlob* blob) const { return
>>>>>>> contains(blob->code_begin()); }
>>>>>>>
>>>>>>> But that my be wrong in the corner case where the size of the
>>>>>>> CodeBlob's payload is zero (i.e. the CodeBlob consists only of the
>>>>>>> 'header' - i.e. the C++ object itself) because in that case
>>>>>>> CodeBlob::code_begin() points right behind the CodeBlob's header
>>>>>>> which
>>>>>>> is a memory location which doesn't belong to the CodeBlob anymore.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> I recall this change was somehow necessary to allow merging
>>>>>> AOTCodeHeap::contains_blob and CodeHead::contains_blob into
>>>>>> one devirtualized method, so you need to ensure all AOT tests
>>>>>> pass with this change (on linux-x64).
>>>>>>
>>>>>
>>>>> All of hotspot/test/aot and hotspot/test/jvmci executed and passed
>>>>> successful. Are there any other tests I should check?
>>>>>
>>>>> That said, it is a little hard to follow the stages of your change. It
>>>>> seems like
>>>>> http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.00/
>>>>> was reviewed [1] but then finally the slightly changed version from
>>>>> http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.01/ was
>>>>> checked in and linked to the bug report.
>>>>>
>>>>> The first, reviewed version of the change still had a correct version
>>>>> of 'CodeHeap::contains_blob(const CodeBlob* blob)' while the second,
>>>>> checked in version has the faulty version of that method.
>>>>>
>>>>> I don't know why you finally did that change to 'contains_blob()' but
>>>>> I don't see any reason why we shouldn't be able to directly use the
>>>>> blob's address for inclusion checking. From what I understand, it
>>>>> should ALWAYS be contained in the corresponding CodeHeap so no reason
>>>>> to mess with 'CodeBlob::code_begin()'.
>>>>>
>>>>> Please let me know if I'm missing something.
>>>>>
>>>>> [1]
>>>>>
>>>>> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2017-July/026624.html
>>>>>
>>>>>> I can't help to wonder if we'd not be better served by disallowing
>>>>>> zero-sized payloads. Is this something that can ever actually
>>>>>> happen except by abuse of the white box API?
>>>>>>
>>>>>
>>>>> The corresponding test (ReturnBlobToWrongHeapTest.java) specifically
>>>>> wants to allocate "segment sized" blocks which is most easily achieved
>>>>> by allocation zero-sized CodeBlobs. And I think there's nothing wrong
>>>>> about it if we handle the inclusion tests correctly.
>>>>>
>>>>> Thank you and best regards,
>>>>> Volker
>>>>>
>>>>>> /Claes
Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8166317: InterpreterCodeSize should be computed

Vladimir Kozlov
On 9/5/17 9:49 AM, Volker Simonis wrote:

> On Fri, Sep 1, 2017 at 6:16 PM, Vladimir Kozlov
> <[hidden email]> wrote:
>> May be add new CodeBlob's method to adjust sizes instead of directly setting
>> them in  CodeCache::free_unused_tail(). Then you would not need friend class
>> CodeCache in CodeBlob.
>>
>
> Changed as suggested (I didn't liked the friend declaration as well :)
>
>> Also I think adjustment to header_size should be done in
>> CodeCache::free_unused_tail() to limit scope of code who knows about blob
>> layout.
>>
>
> Yes, that's much cleaner. Please find the updated webrev here:
>
> http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317.v2/

Good.

>
> I've also found another "day 1" problem in StubQueue::next():
>
>     Stub* next(Stub* s) const                      { int i =
> index_of(s) + stub_size(s);
> -                                                   if (i ==
> _buffer_limit) i = 0;
> +                                                   // Only wrap
> around in the non-contiguous case (see stubss.cpp)
> +                                                   if (i ==
> _buffer_limit && _queue_end < _buffer_limit) i = 0;
>                                                      return (i ==
> _queue_end) ? NULL : stub_at(i);
>                                                    }
>
> The problem was that the method was not prepared to handle the case
> where _buffer_limit == _queue_end == _buffer_size which lead to an
> infinite recursion when iterating over a StubQueue with
> StubQueue::next() until next() returns NULL (as this was for example
> done with -XX:+PrintInterpreter). But with the new, trimmed CodeBlob
> we run into exactly this situation.

Okay.

>
> While doing this last fix I also noticed that "StubQueue::stubs_do()",
> "StubQueue::queues_do()" and "StubQueue::register_queue()" don't seem
> to be used anywhere in the open code base (please correct me if I'm
> wrong). What do you think, maybe we should remove this code in a
> follow up change if it is really not needed?

register_queue() is used in constructor. Other 2 you can remove.
stub_code_begin() and stub_code_end() are not used too -remove.
I thought we run on linux with flag which warn about unused code.

>
> Finally, could you please run the new version through JPRT and sponsor
> it once jdk10/hs will be opened again?

Will do when jdk10 "consolidation" is finished. Please, remind me later
if I forget.

Thanks,
Vladimir

>
> Thanks,
> Volker
>
>> Thanks,
>> Vladimir
>>
>>
>> On 9/1/17 8:46 AM, Volker Simonis wrote:
>>>
>>> Hi,
>>>
>>> I've decided to split the fix for the 'CodeHeap::contains_blob()'
>>> problem into its own issue "8187091: ReturnBlobToWrongHeapTest fails
>>> because of problems in CodeHeap::contains_blob()"
>>> (https://bugs.openjdk.java.net/browse/JDK-8187091) and started a new
>>> review thread for discussing it at:
>>>
>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-September/028206.html
>>>
>>> So please lets keep this thread for discussing the interpreter code
>>> size issue only. I've prepared a new version of the webrev which is
>>> the same as the first one with the only difference that the change to
>>> 'CodeHeap::contains_blob()' has been removed:
>>>
>>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317.v1/
>>>
>>> Thanks,
>>> Volker
>>>
>>>
>>> On Thu, Aug 31, 2017 at 6:35 PM, Volker Simonis
>>> <[hidden email]> wrote:
>>>>
>>>> On Thu, Aug 31, 2017 at 6:05 PM, Vladimir Kozlov
>>>> <[hidden email]> wrote:
>>>>>
>>>>> Very good change. Thank you, Volker.
>>>>>
>>>>> About contains_blob(). The problem is that AOTCompiledMethod allocated
>>>>> in
>>>>> CHeap and not in aot code section (which is RO):
>>>>>
>>>>>
>>>>> http://hg.openjdk.java.net/jdk10/hs/hotspot/file/8acd232fb52a/src/share/vm/aot/aotCompiledMethod.hpp#l124
>>>>>
>>>>> It is allocated in CHeap after AOT library is loaded. Its code_begin()
>>>>> points to AOT code section but AOTCompiledMethod* points outside it (to
>>>>> normal malloced space) so you can't use (char*)blob address.
>>>>>
>>>>
>>>> Thanks for the explanation - now I got it.
>>>>
>>>>> There are 2 ways to fix it, I think.
>>>>> One is to add new field to CodeBlobLayout and set it to blob* address
>>>>> for
>>>>> normal CodeCache blobs and to code_begin for AOT code.
>>>>> Second is to use contains(blob->code_end() - 1) assuming that AOT code
>>>>> is
>>>>> never zero.
>>>>>
>>>>
>>>> I'll give it a try tomorrow and will send out a new webrev.
>>>>
>>>> Regards,
>>>> Volker
>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>>
>>>>> On 8/31/17 5:43 AM, Volker Simonis wrote:
>>>>>>
>>>>>>
>>>>>> On Thu, Aug 31, 2017 at 12:14 PM, Claes Redestad
>>>>>> <[hidden email]> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 2017-08-31 08:54, Volker Simonis wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> While working on this, I found another problem which is related to
>>>>>>>> the
>>>>>>>> fix of JDK-8183573 and leads to crashes when executing the JTreg test
>>>>>>>> compiler/codecache/stress/ReturnBlobToWrongHeapTest.java.
>>>>>>>>
>>>>>>>> The problem is that JDK-8183573 replaced
>>>>>>>>
>>>>>>>>       virtual bool contains_blob(const CodeBlob* blob) const { return
>>>>>>>> low_boundary() <= (char*) blob && (char*) blob < high(); }
>>>>>>>>
>>>>>>>> by:
>>>>>>>>
>>>>>>>>       bool contains_blob(const CodeBlob* blob) const { return
>>>>>>>> contains(blob->code_begin()); }
>>>>>>>>
>>>>>>>> But that my be wrong in the corner case where the size of the
>>>>>>>> CodeBlob's payload is zero (i.e. the CodeBlob consists only of the
>>>>>>>> 'header' - i.e. the C++ object itself) because in that case
>>>>>>>> CodeBlob::code_begin() points right behind the CodeBlob's header
>>>>>>>> which
>>>>>>>> is a memory location which doesn't belong to the CodeBlob anymore.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> I recall this change was somehow necessary to allow merging
>>>>>>> AOTCodeHeap::contains_blob and CodeHead::contains_blob into
>>>>>>> one devirtualized method, so you need to ensure all AOT tests
>>>>>>> pass with this change (on linux-x64).
>>>>>>>
>>>>>>
>>>>>> All of hotspot/test/aot and hotspot/test/jvmci executed and passed
>>>>>> successful. Are there any other tests I should check?
>>>>>>
>>>>>> That said, it is a little hard to follow the stages of your change. It
>>>>>> seems like
>>>>>> http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.00/
>>>>>> was reviewed [1] but then finally the slightly changed version from
>>>>>> http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.01/ was
>>>>>> checked in and linked to the bug report.
>>>>>>
>>>>>> The first, reviewed version of the change still had a correct version
>>>>>> of 'CodeHeap::contains_blob(const CodeBlob* blob)' while the second,
>>>>>> checked in version has the faulty version of that method.
>>>>>>
>>>>>> I don't know why you finally did that change to 'contains_blob()' but
>>>>>> I don't see any reason why we shouldn't be able to directly use the
>>>>>> blob's address for inclusion checking. From what I understand, it
>>>>>> should ALWAYS be contained in the corresponding CodeHeap so no reason
>>>>>> to mess with 'CodeBlob::code_begin()'.
>>>>>>
>>>>>> Please let me know if I'm missing something.
>>>>>>
>>>>>> [1]
>>>>>>
>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2017-July/026624.html
>>>>>>
>>>>>>> I can't help to wonder if we'd not be better served by disallowing
>>>>>>> zero-sized payloads. Is this something that can ever actually
>>>>>>> happen except by abuse of the white box API?
>>>>>>>
>>>>>>
>>>>>> The corresponding test (ReturnBlobToWrongHeapTest.java) specifically
>>>>>> wants to allocate "segment sized" blocks which is most easily achieved
>>>>>> by allocation zero-sized CodeBlobs. And I think there's nothing wrong
>>>>>> about it if we handle the inclusion tests correctly.
>>>>>>
>>>>>> Thank you and best regards,
>>>>>> Volker
>>>>>>
>>>>>>> /Claes
Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8166317: InterpreterCodeSize should be computed

coleen.phillimore

I was going to make the same comment about the friend declaration in v1,
so v2 looks better to me.  Looks good.  Thank you for finding a solution
to this problem that we've had for a long time.  I will sponsor this
(remind me if I forget after the 18th).

thanks,
Coleen


On 9/5/17 1:17 PM, Vladimir Kozlov wrote:

> On 9/5/17 9:49 AM, Volker Simonis wrote:
>> On Fri, Sep 1, 2017 at 6:16 PM, Vladimir Kozlov
>> <[hidden email]> wrote:
>>> May be add new CodeBlob's method to adjust sizes instead of directly
>>> setting
>>> them in  CodeCache::free_unused_tail(). Then you would not need
>>> friend class
>>> CodeCache in CodeBlob.
>>>
>>
>> Changed as suggested (I didn't liked the friend declaration as well :)
>>
>>> Also I think adjustment to header_size should be done in
>>> CodeCache::free_unused_tail() to limit scope of code who knows about
>>> blob
>>> layout.
>>>
>>
>> Yes, that's much cleaner. Please find the updated webrev here:
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317.v2/
>
> Good.
>
>>
>> I've also found another "day 1" problem in StubQueue::next():
>>
>>     Stub* next(Stub* s) const                      { int i =
>> index_of(s) + stub_size(s);
>> -                                                   if (i ==
>> _buffer_limit) i = 0;
>> +                                                   // Only wrap
>> around in the non-contiguous case (see stubss.cpp)
>> +                                                   if (i ==
>> _buffer_limit && _queue_end < _buffer_limit) i = 0;
>>                                                      return (i ==
>> _queue_end) ? NULL : stub_at(i);
>>                                                    }
>>
>> The problem was that the method was not prepared to handle the case
>> where _buffer_limit == _queue_end == _buffer_size which lead to an
>> infinite recursion when iterating over a StubQueue with
>> StubQueue::next() until next() returns NULL (as this was for example
>> done with -XX:+PrintInterpreter). But with the new, trimmed CodeBlob
>> we run into exactly this situation.
>
> Okay.
>
>>
>> While doing this last fix I also noticed that "StubQueue::stubs_do()",
>> "StubQueue::queues_do()" and "StubQueue::register_queue()" don't seem
>> to be used anywhere in the open code base (please correct me if I'm
>> wrong). What do you think, maybe we should remove this code in a
>> follow up change if it is really not needed?
>
> register_queue() is used in constructor. Other 2 you can remove.
> stub_code_begin() and stub_code_end() are not used too -remove.
> I thought we run on linux with flag which warn about unused code.
>
>>
>> Finally, could you please run the new version through JPRT and sponsor
>> it once jdk10/hs will be opened again?
>
> Will do when jdk10 "consolidation" is finished. Please, remind me
> later if I forget.
>
> Thanks,
> Vladimir
>
>>
>> Thanks,
>> Volker
>>
>>> Thanks,
>>> Vladimir
>>>
>>>
>>> On 9/1/17 8:46 AM, Volker Simonis wrote:
>>>>
>>>> Hi,
>>>>
>>>> I've decided to split the fix for the 'CodeHeap::contains_blob()'
>>>> problem into its own issue "8187091: ReturnBlobToWrongHeapTest fails
>>>> because of problems in CodeHeap::contains_blob()"
>>>> (https://bugs.openjdk.java.net/browse/JDK-8187091) and started a new
>>>> review thread for discussing it at:
>>>>
>>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-September/028206.html 
>>>>
>>>>
>>>> So please lets keep this thread for discussing the interpreter code
>>>> size issue only. I've prepared a new version of the webrev which is
>>>> the same as the first one with the only difference that the change to
>>>> 'CodeHeap::contains_blob()' has been removed:
>>>>
>>>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317.v1/
>>>>
>>>> Thanks,
>>>> Volker
>>>>
>>>>
>>>> On Thu, Aug 31, 2017 at 6:35 PM, Volker Simonis
>>>> <[hidden email]> wrote:
>>>>>
>>>>> On Thu, Aug 31, 2017 at 6:05 PM, Vladimir Kozlov
>>>>> <[hidden email]> wrote:
>>>>>>
>>>>>> Very good change. Thank you, Volker.
>>>>>>
>>>>>> About contains_blob(). The problem is that AOTCompiledMethod
>>>>>> allocated
>>>>>> in
>>>>>> CHeap and not in aot code section (which is RO):
>>>>>>
>>>>>>
>>>>>> http://hg.openjdk.java.net/jdk10/hs/hotspot/file/8acd232fb52a/src/share/vm/aot/aotCompiledMethod.hpp#l124 
>>>>>>
>>>>>>
>>>>>> It is allocated in CHeap after AOT library is loaded. Its
>>>>>> code_begin()
>>>>>> points to AOT code section but AOTCompiledMethod* points outside
>>>>>> it (to
>>>>>> normal malloced space) so you can't use (char*)blob address.
>>>>>>
>>>>>
>>>>> Thanks for the explanation - now I got it.
>>>>>
>>>>>> There are 2 ways to fix it, I think.
>>>>>> One is to add new field to CodeBlobLayout and set it to blob*
>>>>>> address
>>>>>> for
>>>>>> normal CodeCache blobs and to code_begin for AOT code.
>>>>>> Second is to use contains(blob->code_end() - 1) assuming that AOT
>>>>>> code
>>>>>> is
>>>>>> never zero.
>>>>>>
>>>>>
>>>>> I'll give it a try tomorrow and will send out a new webrev.
>>>>>
>>>>> Regards,
>>>>> Volker
>>>>>
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>>
>>>>>>
>>>>>> On 8/31/17 5:43 AM, Volker Simonis wrote:
>>>>>>>
>>>>>>>
>>>>>>> On Thu, Aug 31, 2017 at 12:14 PM, Claes Redestad
>>>>>>> <[hidden email]> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2017-08-31 08:54, Volker Simonis wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> While working on this, I found another problem which is
>>>>>>>>> related to
>>>>>>>>> the
>>>>>>>>> fix of JDK-8183573 and leads to crashes when executing the
>>>>>>>>> JTreg test
>>>>>>>>> compiler/codecache/stress/ReturnBlobToWrongHeapTest.java.
>>>>>>>>>
>>>>>>>>> The problem is that JDK-8183573 replaced
>>>>>>>>>
>>>>>>>>>       virtual bool contains_blob(const CodeBlob* blob) const {
>>>>>>>>> return
>>>>>>>>> low_boundary() <= (char*) blob && (char*) blob < high(); }
>>>>>>>>>
>>>>>>>>> by:
>>>>>>>>>
>>>>>>>>>       bool contains_blob(const CodeBlob* blob) const { return
>>>>>>>>> contains(blob->code_begin()); }
>>>>>>>>>
>>>>>>>>> But that my be wrong in the corner case where the size of the
>>>>>>>>> CodeBlob's payload is zero (i.e. the CodeBlob consists only of
>>>>>>>>> the
>>>>>>>>> 'header' - i.e. the C++ object itself) because in that case
>>>>>>>>> CodeBlob::code_begin() points right behind the CodeBlob's header
>>>>>>>>> which
>>>>>>>>> is a memory location which doesn't belong to the CodeBlob
>>>>>>>>> anymore.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> I recall this change was somehow necessary to allow merging
>>>>>>>> AOTCodeHeap::contains_blob and CodeHead::contains_blob into
>>>>>>>> one devirtualized method, so you need to ensure all AOT tests
>>>>>>>> pass with this change (on linux-x64).
>>>>>>>>
>>>>>>>
>>>>>>> All of hotspot/test/aot and hotspot/test/jvmci executed and passed
>>>>>>> successful. Are there any other tests I should check?
>>>>>>>
>>>>>>> That said, it is a little hard to follow the stages of your
>>>>>>> change. It
>>>>>>> seems like
>>>>>>> http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.00/
>>>>>>> was reviewed [1] but then finally the slightly changed version from
>>>>>>> http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.01/ 
>>>>>>> was
>>>>>>> checked in and linked to the bug report.
>>>>>>>
>>>>>>> The first, reviewed version of the change still had a correct
>>>>>>> version
>>>>>>> of 'CodeHeap::contains_blob(const CodeBlob* blob)' while the
>>>>>>> second,
>>>>>>> checked in version has the faulty version of that method.
>>>>>>>
>>>>>>> I don't know why you finally did that change to
>>>>>>> 'contains_blob()' but
>>>>>>> I don't see any reason why we shouldn't be able to directly use the
>>>>>>> blob's address for inclusion checking. From what I understand, it
>>>>>>> should ALWAYS be contained in the corresponding CodeHeap so no
>>>>>>> reason
>>>>>>> to mess with 'CodeBlob::code_begin()'.
>>>>>>>
>>>>>>> Please let me know if I'm missing something.
>>>>>>>
>>>>>>> [1]
>>>>>>>
>>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2017-July/026624.html 
>>>>>>>
>>>>>>>
>>>>>>>> I can't help to wonder if we'd not be better served by disallowing
>>>>>>>> zero-sized payloads. Is this something that can ever actually
>>>>>>>> happen except by abuse of the white box API?
>>>>>>>>
>>>>>>>
>>>>>>> The corresponding test (ReturnBlobToWrongHeapTest.java)
>>>>>>> specifically
>>>>>>> wants to allocate "segment sized" blocks which is most easily
>>>>>>> achieved
>>>>>>> by allocation zero-sized CodeBlobs. And I think there's nothing
>>>>>>> wrong
>>>>>>> about it if we handle the inclusion tests correctly.
>>>>>>>
>>>>>>> Thank you and best regards,
>>>>>>> Volker
>>>>>>>
>>>>>>>> /Claes

Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8166317: InterpreterCodeSize should be computed

coleen.phillimore
In reply to this post by Vladimir Kozlov


On 9/5/17 1:17 PM, Vladimir Kozlov wrote:
>>
>> Finally, could you please run the new version through JPRT and sponsor
>> it once jdk10/hs will be opened again?
>
> Will do when jdk10 "consolidation" is finished. Please, remind me
> later if I forget.
I didn't see this - Vladimir, you can sponsor.
Thanks,
Coleen
Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8166317: InterpreterCodeSize should be computed

Volker Simonis
In reply to this post by coleen.phillimore
On Tue, Sep 5, 2017 at 9:36 PM,  <[hidden email]> wrote:
>
> I was going to make the same comment about the friend declaration in v1, so
> v2 looks better to me.  Looks good.  Thank you for finding a solution to
> this problem that we've had for a long time.  I will sponsor this (remind me
> if I forget after the 18th).
>

Thanks Coleen! I've updated

http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317.v2/

in-place and added you as a second reviewer.

Regards,
Volker


> thanks,
> Coleen
>
>
>
> On 9/5/17 1:17 PM, Vladimir Kozlov wrote:
>>
>> On 9/5/17 9:49 AM, Volker Simonis wrote:
>>>
>>> On Fri, Sep 1, 2017 at 6:16 PM, Vladimir Kozlov
>>> <[hidden email]> wrote:
>>>>
>>>> May be add new CodeBlob's method to adjust sizes instead of directly
>>>> setting
>>>> them in  CodeCache::free_unused_tail(). Then you would not need friend
>>>> class
>>>> CodeCache in CodeBlob.
>>>>
>>>
>>> Changed as suggested (I didn't liked the friend declaration as well :)
>>>
>>>> Also I think adjustment to header_size should be done in
>>>> CodeCache::free_unused_tail() to limit scope of code who knows about
>>>> blob
>>>> layout.
>>>>
>>>
>>> Yes, that's much cleaner. Please find the updated webrev here:
>>>
>>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317.v2/
>>
>>
>> Good.
>>
>>>
>>> I've also found another "day 1" problem in StubQueue::next():
>>>
>>>     Stub* next(Stub* s) const                      { int i =
>>> index_of(s) + stub_size(s);
>>> -                                                   if (i ==
>>> _buffer_limit) i = 0;
>>> +                                                   // Only wrap
>>> around in the non-contiguous case (see stubss.cpp)
>>> +                                                   if (i ==
>>> _buffer_limit && _queue_end < _buffer_limit) i = 0;
>>>                                                      return (i ==
>>> _queue_end) ? NULL : stub_at(i);
>>>                                                    }
>>>
>>> The problem was that the method was not prepared to handle the case
>>> where _buffer_limit == _queue_end == _buffer_size which lead to an
>>> infinite recursion when iterating over a StubQueue with
>>> StubQueue::next() until next() returns NULL (as this was for example
>>> done with -XX:+PrintInterpreter). But with the new, trimmed CodeBlob
>>> we run into exactly this situation.
>>
>>
>> Okay.
>>
>>>
>>> While doing this last fix I also noticed that "StubQueue::stubs_do()",
>>> "StubQueue::queues_do()" and "StubQueue::register_queue()" don't seem
>>> to be used anywhere in the open code base (please correct me if I'm
>>> wrong). What do you think, maybe we should remove this code in a
>>> follow up change if it is really not needed?
>>
>>
>> register_queue() is used in constructor. Other 2 you can remove.
>> stub_code_begin() and stub_code_end() are not used too -remove.
>> I thought we run on linux with flag which warn about unused code.
>>
>>>
>>> Finally, could you please run the new version through JPRT and sponsor
>>> it once jdk10/hs will be opened again?
>>
>>
>> Will do when jdk10 "consolidation" is finished. Please, remind me later if
>> I forget.
>>
>> Thanks,
>> Vladimir
>>
>>>
>>> Thanks,
>>> Volker
>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>>
>>>> On 9/1/17 8:46 AM, Volker Simonis wrote:
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> I've decided to split the fix for the 'CodeHeap::contains_blob()'
>>>>> problem into its own issue "8187091: ReturnBlobToWrongHeapTest fails
>>>>> because of problems in CodeHeap::contains_blob()"
>>>>> (https://bugs.openjdk.java.net/browse/JDK-8187091) and started a new
>>>>> review thread for discussing it at:
>>>>>
>>>>>
>>>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-September/028206.html
>>>>>
>>>>> So please lets keep this thread for discussing the interpreter code
>>>>> size issue only. I've prepared a new version of the webrev which is
>>>>> the same as the first one with the only difference that the change to
>>>>> 'CodeHeap::contains_blob()' has been removed:
>>>>>
>>>>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317.v1/
>>>>>
>>>>> Thanks,
>>>>> Volker
>>>>>
>>>>>
>>>>> On Thu, Aug 31, 2017 at 6:35 PM, Volker Simonis
>>>>> <[hidden email]> wrote:
>>>>>>
>>>>>>
>>>>>> On Thu, Aug 31, 2017 at 6:05 PM, Vladimir Kozlov
>>>>>> <[hidden email]> wrote:
>>>>>>>
>>>>>>>
>>>>>>> Very good change. Thank you, Volker.
>>>>>>>
>>>>>>> About contains_blob(). The problem is that AOTCompiledMethod
>>>>>>> allocated
>>>>>>> in
>>>>>>> CHeap and not in aot code section (which is RO):
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> http://hg.openjdk.java.net/jdk10/hs/hotspot/file/8acd232fb52a/src/share/vm/aot/aotCompiledMethod.hpp#l124
>>>>>>>
>>>>>>> It is allocated in CHeap after AOT library is loaded. Its
>>>>>>> code_begin()
>>>>>>> points to AOT code section but AOTCompiledMethod* points outside it
>>>>>>> (to
>>>>>>> normal malloced space) so you can't use (char*)blob address.
>>>>>>>
>>>>>>
>>>>>> Thanks for the explanation - now I got it.
>>>>>>
>>>>>>> There are 2 ways to fix it, I think.
>>>>>>> One is to add new field to CodeBlobLayout and set it to blob* address
>>>>>>> for
>>>>>>> normal CodeCache blobs and to code_begin for AOT code.
>>>>>>> Second is to use contains(blob->code_end() - 1) assuming that AOT
>>>>>>> code
>>>>>>> is
>>>>>>> never zero.
>>>>>>>
>>>>>>
>>>>>> I'll give it a try tomorrow and will send out a new webrev.
>>>>>>
>>>>>> Regards,
>>>>>> Volker
>>>>>>
>>>>>>> Thanks,
>>>>>>> Vladimir
>>>>>>>
>>>>>>>
>>>>>>> On 8/31/17 5:43 AM, Volker Simonis wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On Thu, Aug 31, 2017 at 12:14 PM, Claes Redestad
>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2017-08-31 08:54, Volker Simonis wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> While working on this, I found another problem which is related to
>>>>>>>>>> the
>>>>>>>>>> fix of JDK-8183573 and leads to crashes when executing the JTreg
>>>>>>>>>> test
>>>>>>>>>> compiler/codecache/stress/ReturnBlobToWrongHeapTest.java.
>>>>>>>>>>
>>>>>>>>>> The problem is that JDK-8183573 replaced
>>>>>>>>>>
>>>>>>>>>>       virtual bool contains_blob(const CodeBlob* blob) const {
>>>>>>>>>> return
>>>>>>>>>> low_boundary() <= (char*) blob && (char*) blob < high(); }
>>>>>>>>>>
>>>>>>>>>> by:
>>>>>>>>>>
>>>>>>>>>>       bool contains_blob(const CodeBlob* blob) const { return
>>>>>>>>>> contains(blob->code_begin()); }
>>>>>>>>>>
>>>>>>>>>> But that my be wrong in the corner case where the size of the
>>>>>>>>>> CodeBlob's payload is zero (i.e. the CodeBlob consists only of the
>>>>>>>>>> 'header' - i.e. the C++ object itself) because in that case
>>>>>>>>>> CodeBlob::code_begin() points right behind the CodeBlob's header
>>>>>>>>>> which
>>>>>>>>>> is a memory location which doesn't belong to the CodeBlob anymore.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I recall this change was somehow necessary to allow merging
>>>>>>>>> AOTCodeHeap::contains_blob and CodeHead::contains_blob into
>>>>>>>>> one devirtualized method, so you need to ensure all AOT tests
>>>>>>>>> pass with this change (on linux-x64).
>>>>>>>>>
>>>>>>>>
>>>>>>>> All of hotspot/test/aot and hotspot/test/jvmci executed and passed
>>>>>>>> successful. Are there any other tests I should check?
>>>>>>>>
>>>>>>>> That said, it is a little hard to follow the stages of your change.
>>>>>>>> It
>>>>>>>> seems like
>>>>>>>> http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.00/
>>>>>>>> was reviewed [1] but then finally the slightly changed version from
>>>>>>>> http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.01/
>>>>>>>> was
>>>>>>>> checked in and linked to the bug report.
>>>>>>>>
>>>>>>>> The first, reviewed version of the change still had a correct
>>>>>>>> version
>>>>>>>> of 'CodeHeap::contains_blob(const CodeBlob* blob)' while the second,
>>>>>>>> checked in version has the faulty version of that method.
>>>>>>>>
>>>>>>>> I don't know why you finally did that change to 'contains_blob()'
>>>>>>>> but
>>>>>>>> I don't see any reason why we shouldn't be able to directly use the
>>>>>>>> blob's address for inclusion checking. From what I understand, it
>>>>>>>> should ALWAYS be contained in the corresponding CodeHeap so no
>>>>>>>> reason
>>>>>>>> to mess with 'CodeBlob::code_begin()'.
>>>>>>>>
>>>>>>>> Please let me know if I'm missing something.
>>>>>>>>
>>>>>>>> [1]
>>>>>>>>
>>>>>>>>
>>>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2017-July/026624.html
>>>>>>>>
>>>>>>>>> I can't help to wonder if we'd not be better served by disallowing
>>>>>>>>> zero-sized payloads. Is this something that can ever actually
>>>>>>>>> happen except by abuse of the white box API?
>>>>>>>>>
>>>>>>>>
>>>>>>>> The corresponding test (ReturnBlobToWrongHeapTest.java) specifically
>>>>>>>> wants to allocate "segment sized" blocks which is most easily
>>>>>>>> achieved
>>>>>>>> by allocation zero-sized CodeBlobs. And I think there's nothing
>>>>>>>> wrong
>>>>>>>> about it if we handle the inclusion tests correctly.
>>>>>>>>
>>>>>>>> Thank you and best regards,
>>>>>>>> Volker
>>>>>>>>
>>>>>>>>> /Claes
>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8166317: InterpreterCodeSize should be computed

Volker Simonis
In reply to this post by Vladimir Kozlov
On Tue, Sep 5, 2017 at 7:17 PM, Vladimir Kozlov
<[hidden email]> wrote:

> On 9/5/17 9:49 AM, Volker Simonis wrote:
>>
>> On Fri, Sep 1, 2017 at 6:16 PM, Vladimir Kozlov
>> <[hidden email]> wrote:
>>>
>>> May be add new CodeBlob's method to adjust sizes instead of directly
>>> setting
>>> them in  CodeCache::free_unused_tail(). Then you would not need friend
>>> class
>>> CodeCache in CodeBlob.
>>>
>>
>> Changed as suggested (I didn't liked the friend declaration as well :)
>>
>>> Also I think adjustment to header_size should be done in
>>> CodeCache::free_unused_tail() to limit scope of code who knows about blob
>>> layout.
>>>
>>
>> Yes, that's much cleaner. Please find the updated webrev here:
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317.v2/
>
>
> Good.
>
>>
>> I've also found another "day 1" problem in StubQueue::next():
>>
>>     Stub* next(Stub* s) const                      { int i =
>> index_of(s) + stub_size(s);
>> -                                                   if (i ==
>> _buffer_limit) i = 0;
>> +                                                   // Only wrap
>> around in the non-contiguous case (see stubss.cpp)
>> +                                                   if (i ==
>> _buffer_limit && _queue_end < _buffer_limit) i = 0;
>>                                                      return (i ==
>> _queue_end) ? NULL : stub_at(i);
>>                                                    }
>>
>> The problem was that the method was not prepared to handle the case
>> where _buffer_limit == _queue_end == _buffer_size which lead to an
>> infinite recursion when iterating over a StubQueue with
>> StubQueue::next() until next() returns NULL (as this was for example
>> done with -XX:+PrintInterpreter). But with the new, trimmed CodeBlob
>> we run into exactly this situation.
>
>
> Okay.
>
>>
>> While doing this last fix I also noticed that "StubQueue::stubs_do()",
>> "StubQueue::queues_do()" and "StubQueue::register_queue()" don't seem
>> to be used anywhere in the open code base (please correct me if I'm
>> wrong). What do you think, maybe we should remove this code in a
>> follow up change if it is really not needed?
>
>
> register_queue() is used in constructor. Other 2 you can remove.
> stub_code_begin() and stub_code_end() are not used too -remove.
> I thought we run on linux with flag which warn about unused code.
>

Yes, but register_queue() is only required for iterating over all
queues with queues_do() so we can remove it as well if we remove
queues_do().

I've opened "8187280: Remove unused methods from StubQueue" for this
and started a new review thread at:

http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-September/028283.html

Please let's follow up on this issue there.

>>
>> Finally, could you please run the new version through JPRT and sponsor
>> it once jdk10/hs will be opened again?
>
>
> Will do when jdk10 "consolidation" is finished. Please, remind me later if I
> forget.
>

Thanks,
Volker

> Thanks,
> Vladimir
>
>
>>
>> Thanks,
>> Volker
>>
>>> Thanks,
>>> Vladimir
>>>
>>>
>>> On 9/1/17 8:46 AM, Volker Simonis wrote:
>>>>
>>>>
>>>> Hi,
>>>>
>>>> I've decided to split the fix for the 'CodeHeap::contains_blob()'
>>>> problem into its own issue "8187091: ReturnBlobToWrongHeapTest fails
>>>> because of problems in CodeHeap::contains_blob()"
>>>> (https://bugs.openjdk.java.net/browse/JDK-8187091) and started a new
>>>> review thread for discussing it at:
>>>>
>>>>
>>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-September/028206.html
>>>>
>>>> So please lets keep this thread for discussing the interpreter code
>>>> size issue only. I've prepared a new version of the webrev which is
>>>> the same as the first one with the only difference that the change to
>>>> 'CodeHeap::contains_blob()' has been removed:
>>>>
>>>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317.v1/
>>>>
>>>> Thanks,
>>>> Volker
>>>>
>>>>
>>>> On Thu, Aug 31, 2017 at 6:35 PM, Volker Simonis
>>>> <[hidden email]> wrote:
>>>>>
>>>>>
>>>>> On Thu, Aug 31, 2017 at 6:05 PM, Vladimir Kozlov
>>>>> <[hidden email]> wrote:
>>>>>>
>>>>>>
>>>>>> Very good change. Thank you, Volker.
>>>>>>
>>>>>> About contains_blob(). The problem is that AOTCompiledMethod allocated
>>>>>> in
>>>>>> CHeap and not in aot code section (which is RO):
>>>>>>
>>>>>>
>>>>>>
>>>>>> http://hg.openjdk.java.net/jdk10/hs/hotspot/file/8acd232fb52a/src/share/vm/aot/aotCompiledMethod.hpp#l124
>>>>>>
>>>>>> It is allocated in CHeap after AOT library is loaded. Its code_begin()
>>>>>> points to AOT code section but AOTCompiledMethod* points outside it
>>>>>> (to
>>>>>> normal malloced space) so you can't use (char*)blob address.
>>>>>>
>>>>>
>>>>> Thanks for the explanation - now I got it.
>>>>>
>>>>>> There are 2 ways to fix it, I think.
>>>>>> One is to add new field to CodeBlobLayout and set it to blob* address
>>>>>> for
>>>>>> normal CodeCache blobs and to code_begin for AOT code.
>>>>>> Second is to use contains(blob->code_end() - 1) assuming that AOT code
>>>>>> is
>>>>>> never zero.
>>>>>>
>>>>>
>>>>> I'll give it a try tomorrow and will send out a new webrev.
>>>>>
>>>>> Regards,
>>>>> Volker
>>>>>
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>>
>>>>>>
>>>>>> On 8/31/17 5:43 AM, Volker Simonis wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Thu, Aug 31, 2017 at 12:14 PM, Claes Redestad
>>>>>>> <[hidden email]> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2017-08-31 08:54, Volker Simonis wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> While working on this, I found another problem which is related to
>>>>>>>>> the
>>>>>>>>> fix of JDK-8183573 and leads to crashes when executing the JTreg
>>>>>>>>> test
>>>>>>>>> compiler/codecache/stress/ReturnBlobToWrongHeapTest.java.
>>>>>>>>>
>>>>>>>>> The problem is that JDK-8183573 replaced
>>>>>>>>>
>>>>>>>>>       virtual bool contains_blob(const CodeBlob* blob) const {
>>>>>>>>> return
>>>>>>>>> low_boundary() <= (char*) blob && (char*) blob < high(); }
>>>>>>>>>
>>>>>>>>> by:
>>>>>>>>>
>>>>>>>>>       bool contains_blob(const CodeBlob* blob) const { return
>>>>>>>>> contains(blob->code_begin()); }
>>>>>>>>>
>>>>>>>>> But that my be wrong in the corner case where the size of the
>>>>>>>>> CodeBlob's payload is zero (i.e. the CodeBlob consists only of the
>>>>>>>>> 'header' - i.e. the C++ object itself) because in that case
>>>>>>>>> CodeBlob::code_begin() points right behind the CodeBlob's header
>>>>>>>>> which
>>>>>>>>> is a memory location which doesn't belong to the CodeBlob anymore.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> I recall this change was somehow necessary to allow merging
>>>>>>>> AOTCodeHeap::contains_blob and CodeHead::contains_blob into
>>>>>>>> one devirtualized method, so you need to ensure all AOT tests
>>>>>>>> pass with this change (on linux-x64).
>>>>>>>>
>>>>>>>
>>>>>>> All of hotspot/test/aot and hotspot/test/jvmci executed and passed
>>>>>>> successful. Are there any other tests I should check?
>>>>>>>
>>>>>>> That said, it is a little hard to follow the stages of your change.
>>>>>>> It
>>>>>>> seems like
>>>>>>> http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.00/
>>>>>>> was reviewed [1] but then finally the slightly changed version from
>>>>>>> http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.01/
>>>>>>> was
>>>>>>> checked in and linked to the bug report.
>>>>>>>
>>>>>>> The first, reviewed version of the change still had a correct version
>>>>>>> of 'CodeHeap::contains_blob(const CodeBlob* blob)' while the second,
>>>>>>> checked in version has the faulty version of that method.
>>>>>>>
>>>>>>> I don't know why you finally did that change to 'contains_blob()' but
>>>>>>> I don't see any reason why we shouldn't be able to directly use the
>>>>>>> blob's address for inclusion checking. From what I understand, it
>>>>>>> should ALWAYS be contained in the corresponding CodeHeap so no reason
>>>>>>> to mess with 'CodeBlob::code_begin()'.
>>>>>>>
>>>>>>> Please let me know if I'm missing something.
>>>>>>>
>>>>>>> [1]
>>>>>>>
>>>>>>>
>>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2017-July/026624.html
>>>>>>>
>>>>>>>> I can't help to wonder if we'd not be better served by disallowing
>>>>>>>> zero-sized payloads. Is this something that can ever actually
>>>>>>>> happen except by abuse of the white box API?
>>>>>>>>
>>>>>>>
>>>>>>> The corresponding test (ReturnBlobToWrongHeapTest.java) specifically
>>>>>>> wants to allocate "segment sized" blocks which is most easily
>>>>>>> achieved
>>>>>>> by allocation zero-sized CodeBlobs. And I think there's nothing wrong
>>>>>>> about it if we handle the inclusion tests correctly.
>>>>>>>
>>>>>>> Thank you and best regards,
>>>>>>> Volker
>>>>>>>
>>>>>>>> /Claes
Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8166317: InterpreterCodeSize should be computed

Vladimir Kozlov
In reply to this post by Volker Simonis
I hit build failure when tried to push changes:

src/hotspot/share/code/codeBlob.hpp(162) : warning C4267: '=' : conversion from 'size_t' to 'int', possible loss of data
src/hotspot/share/code/codeBlob.hpp(163) : warning C4267: '=' : conversion from 'size_t' to 'int', possible loss of data

I am going to fix it by casting (int):

+  void adjust_size(size_t used) {
+    _size = (int)used;
+    _data_offset = (int)used;
+    _code_end = (address)this + used;
+    _data_end = (address)this + used;
+  }

Note, CodeCache size can't more than 2Gb (max_int) so such casting is fine.

Vladimir

On 9/6/17 6:20 AM, Volker Simonis wrote:

> On Tue, Sep 5, 2017 at 9:36 PM,  <[hidden email]> wrote:
>>
>> I was going to make the same comment about the friend declaration in v1, so
>> v2 looks better to me.  Looks good.  Thank you for finding a solution to
>> this problem that we've had for a long time.  I will sponsor this (remind me
>> if I forget after the 18th).
>>
>
> Thanks Coleen! I've updated
>
> http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317.v2/
>
> in-place and added you as a second reviewer.
>
> Regards,
> Volker
>
>
>> thanks,
>> Coleen
>>
>>
>>
>> On 9/5/17 1:17 PM, Vladimir Kozlov wrote:
>>>
>>> On 9/5/17 9:49 AM, Volker Simonis wrote:
>>>>
>>>> On Fri, Sep 1, 2017 at 6:16 PM, Vladimir Kozlov
>>>> <[hidden email]> wrote:
>>>>>
>>>>> May be add new CodeBlob's method to adjust sizes instead of directly
>>>>> setting
>>>>> them in  CodeCache::free_unused_tail(). Then you would not need friend
>>>>> class
>>>>> CodeCache in CodeBlob.
>>>>>
>>>>
>>>> Changed as suggested (I didn't liked the friend declaration as well :)
>>>>
>>>>> Also I think adjustment to header_size should be done in
>>>>> CodeCache::free_unused_tail() to limit scope of code who knows about
>>>>> blob
>>>>> layout.
>>>>>
>>>>
>>>> Yes, that's much cleaner. Please find the updated webrev here:
>>>>
>>>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317.v2/
>>>
>>>
>>> Good.
>>>
>>>>
>>>> I've also found another "day 1" problem in StubQueue::next():
>>>>
>>>>      Stub* next(Stub* s) const                      { int i =
>>>> index_of(s) + stub_size(s);
>>>> -                                                   if (i ==
>>>> _buffer_limit) i = 0;
>>>> +                                                   // Only wrap
>>>> around in the non-contiguous case (see stubss.cpp)
>>>> +                                                   if (i ==
>>>> _buffer_limit && _queue_end < _buffer_limit) i = 0;
>>>>                                                       return (i ==
>>>> _queue_end) ? NULL : stub_at(i);
>>>>                                                     }
>>>>
>>>> The problem was that the method was not prepared to handle the case
>>>> where _buffer_limit == _queue_end == _buffer_size which lead to an
>>>> infinite recursion when iterating over a StubQueue with
>>>> StubQueue::next() until next() returns NULL (as this was for example
>>>> done with -XX:+PrintInterpreter). But with the new, trimmed CodeBlob
>>>> we run into exactly this situation.
>>>
>>>
>>> Okay.
>>>
>>>>
>>>> While doing this last fix I also noticed that "StubQueue::stubs_do()",
>>>> "StubQueue::queues_do()" and "StubQueue::register_queue()" don't seem
>>>> to be used anywhere in the open code base (please correct me if I'm
>>>> wrong). What do you think, maybe we should remove this code in a
>>>> follow up change if it is really not needed?
>>>
>>>
>>> register_queue() is used in constructor. Other 2 you can remove.
>>> stub_code_begin() and stub_code_end() are not used too -remove.
>>> I thought we run on linux with flag which warn about unused code.
>>>
>>>>
>>>> Finally, could you please run the new version through JPRT and sponsor
>>>> it once jdk10/hs will be opened again?
>>>
>>>
>>> Will do when jdk10 "consolidation" is finished. Please, remind me later if
>>> I forget.
>>>
>>> Thanks,
>>> Vladimir
>>>
>>>>
>>>> Thanks,
>>>> Volker
>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>>
>>>>> On 9/1/17 8:46 AM, Volker Simonis wrote:
>>>>>>
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> I've decided to split the fix for the 'CodeHeap::contains_blob()'
>>>>>> problem into its own issue "8187091: ReturnBlobToWrongHeapTest fails
>>>>>> because of problems in CodeHeap::contains_blob()"
>>>>>> (https://bugs.openjdk.java.net/browse/JDK-8187091) and started a new
>>>>>> review thread for discussing it at:
>>>>>>
>>>>>>
>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-September/028206.html
>>>>>>
>>>>>> So please lets keep this thread for discussing the interpreter code
>>>>>> size issue only. I've prepared a new version of the webrev which is
>>>>>> the same as the first one with the only difference that the change to
>>>>>> 'CodeHeap::contains_blob()' has been removed:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317.v1/
>>>>>>
>>>>>> Thanks,
>>>>>> Volker
>>>>>>
>>>>>>
>>>>>> On Thu, Aug 31, 2017 at 6:35 PM, Volker Simonis
>>>>>> <[hidden email]> wrote:
>>>>>>>
>>>>>>>
>>>>>>> On Thu, Aug 31, 2017 at 6:05 PM, Vladimir Kozlov
>>>>>>> <[hidden email]> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> Very good change. Thank you, Volker.
>>>>>>>>
>>>>>>>> About contains_blob(). The problem is that AOTCompiledMethod
>>>>>>>> allocated
>>>>>>>> in
>>>>>>>> CHeap and not in aot code section (which is RO):
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> http://hg.openjdk.java.net/jdk10/hs/hotspot/file/8acd232fb52a/src/share/vm/aot/aotCompiledMethod.hpp#l124
>>>>>>>>
>>>>>>>> It is allocated in CHeap after AOT library is loaded. Its
>>>>>>>> code_begin()
>>>>>>>> points to AOT code section but AOTCompiledMethod* points outside it
>>>>>>>> (to
>>>>>>>> normal malloced space) so you can't use (char*)blob address.
>>>>>>>>
>>>>>>>
>>>>>>> Thanks for the explanation - now I got it.
>>>>>>>
>>>>>>>> There are 2 ways to fix it, I think.
>>>>>>>> One is to add new field to CodeBlobLayout and set it to blob* address
>>>>>>>> for
>>>>>>>> normal CodeCache blobs and to code_begin for AOT code.
>>>>>>>> Second is to use contains(blob->code_end() - 1) assuming that AOT
>>>>>>>> code
>>>>>>>> is
>>>>>>>> never zero.
>>>>>>>>
>>>>>>>
>>>>>>> I'll give it a try tomorrow and will send out a new webrev.
>>>>>>>
>>>>>>> Regards,
>>>>>>> Volker
>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Vladimir
>>>>>>>>
>>>>>>>>
>>>>>>>> On 8/31/17 5:43 AM, Volker Simonis wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Thu, Aug 31, 2017 at 12:14 PM, Claes Redestad
>>>>>>>>> <[hidden email]> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 2017-08-31 08:54, Volker Simonis wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> While working on this, I found another problem which is related to
>>>>>>>>>>> the
>>>>>>>>>>> fix of JDK-8183573 and leads to crashes when executing the JTreg
>>>>>>>>>>> test
>>>>>>>>>>> compiler/codecache/stress/ReturnBlobToWrongHeapTest.java.
>>>>>>>>>>>
>>>>>>>>>>> The problem is that JDK-8183573 replaced
>>>>>>>>>>>
>>>>>>>>>>>        virtual bool contains_blob(const CodeBlob* blob) const {
>>>>>>>>>>> return
>>>>>>>>>>> low_boundary() <= (char*) blob && (char*) blob < high(); }
>>>>>>>>>>>
>>>>>>>>>>> by:
>>>>>>>>>>>
>>>>>>>>>>>        bool contains_blob(const CodeBlob* blob) const { return
>>>>>>>>>>> contains(blob->code_begin()); }
>>>>>>>>>>>
>>>>>>>>>>> But that my be wrong in the corner case where the size of the
>>>>>>>>>>> CodeBlob's payload is zero (i.e. the CodeBlob consists only of the
>>>>>>>>>>> 'header' - i.e. the C++ object itself) because in that case
>>>>>>>>>>> CodeBlob::code_begin() points right behind the CodeBlob's header
>>>>>>>>>>> which
>>>>>>>>>>> is a memory location which doesn't belong to the CodeBlob anymore.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I recall this change was somehow necessary to allow merging
>>>>>>>>>> AOTCodeHeap::contains_blob and CodeHead::contains_blob into
>>>>>>>>>> one devirtualized method, so you need to ensure all AOT tests
>>>>>>>>>> pass with this change (on linux-x64).
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> All of hotspot/test/aot and hotspot/test/jvmci executed and passed
>>>>>>>>> successful. Are there any other tests I should check?
>>>>>>>>>
>>>>>>>>> That said, it is a little hard to follow the stages of your change.
>>>>>>>>> It
>>>>>>>>> seems like
>>>>>>>>> http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.00/
>>>>>>>>> was reviewed [1] but then finally the slightly changed version from
>>>>>>>>> http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.01/
>>>>>>>>> was
>>>>>>>>> checked in and linked to the bug report.
>>>>>>>>>
>>>>>>>>> The first, reviewed version of the change still had a correct
>>>>>>>>> version
>>>>>>>>> of 'CodeHeap::contains_blob(const CodeBlob* blob)' while the second,
>>>>>>>>> checked in version has the faulty version of that method.
>>>>>>>>>
>>>>>>>>> I don't know why you finally did that change to 'contains_blob()'
>>>>>>>>> but
>>>>>>>>> I don't see any reason why we shouldn't be able to directly use the
>>>>>>>>> blob's address for inclusion checking. From what I understand, it
>>>>>>>>> should ALWAYS be contained in the corresponding CodeHeap so no
>>>>>>>>> reason
>>>>>>>>> to mess with 'CodeBlob::code_begin()'.
>>>>>>>>>
>>>>>>>>> Please let me know if I'm missing something.
>>>>>>>>>
>>>>>>>>> [1]
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2017-July/026624.html
>>>>>>>>>
>>>>>>>>>> I can't help to wonder if we'd not be better served by disallowing
>>>>>>>>>> zero-sized payloads. Is this something that can ever actually
>>>>>>>>>> happen except by abuse of the white box API?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> The corresponding test (ReturnBlobToWrongHeapTest.java) specifically
>>>>>>>>> wants to allocate "segment sized" blocks which is most easily
>>>>>>>>> achieved
>>>>>>>>> by allocation zero-sized CodeBlobs. And I think there's nothing
>>>>>>>>> wrong
>>>>>>>>> about it if we handle the inclusion tests correctly.
>>>>>>>>>
>>>>>>>>> Thank you and best regards,
>>>>>>>>> Volker
>>>>>>>>>
>>>>>>>>>> /Claes
>>
>>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8166317: InterpreterCodeSize should be computed

Volker Simonis
Hi Vladimir,

thanks a lot for remembering these changes!

Regards,
Volker


Vladimir Kozlov <[hidden email]> schrieb am Fr. 29. Sep. 2017
um 15:47:

> I hit build failure when tried to push changes:
>
> src/hotspot/share/code/codeBlob.hpp(162) : warning C4267: '=' : conversion
> from 'size_t' to 'int', possible loss of data
> src/hotspot/share/code/codeBlob.hpp(163) : warning C4267: '=' : conversion
> from 'size_t' to 'int', possible loss of data
>
> I am going to fix it by casting (int):
>
> +  void adjust_size(size_t used) {
> +    _size = (int)used;
> +    _data_offset = (int)used;
> +    _code_end = (address)this + used;
> +    _data_end = (address)this + used;
> +  }
>
> Note, CodeCache size can't more than 2Gb (max_int) so such casting is fine.
>
> Vladimir
>
> On 9/6/17 6:20 AM, Volker Simonis wrote:
> > On Tue, Sep 5, 2017 at 9:36 PM,  <[hidden email]> wrote:
> >>
> >> I was going to make the same comment about the friend declaration in
> v1, so
> >> v2 looks better to me.  Looks good.  Thank you for finding a solution to
> >> this problem that we've had for a long time.  I will sponsor this
> (remind me
> >> if I forget after the 18th).
> >>
> >
> > Thanks Coleen! I've updated
> >
> > http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317.v2/
> >
> > in-place and added you as a second reviewer.
> >
> > Regards,
> > Volker
> >
> >
> >> thanks,
> >> Coleen
> >>
> >>
> >>
> >> On 9/5/17 1:17 PM, Vladimir Kozlov wrote:
> >>>
> >>> On 9/5/17 9:49 AM, Volker Simonis wrote:
> >>>>
> >>>> On Fri, Sep 1, 2017 at 6:16 PM, Vladimir Kozlov
> >>>> <[hidden email]> wrote:
> >>>>>
> >>>>> May be add new CodeBlob's method to adjust sizes instead of directly
> >>>>> setting
> >>>>> them in  CodeCache::free_unused_tail(). Then you would not need
> friend
> >>>>> class
> >>>>> CodeCache in CodeBlob.
> >>>>>
> >>>>
> >>>> Changed as suggested (I didn't liked the friend declaration as well :)
> >>>>
> >>>>> Also I think adjustment to header_size should be done in
> >>>>> CodeCache::free_unused_tail() to limit scope of code who knows about
> >>>>> blob
> >>>>> layout.
> >>>>>
> >>>>
> >>>> Yes, that's much cleaner. Please find the updated webrev here:
> >>>>
> >>>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317.v2/
> >>>
> >>>
> >>> Good.
> >>>
> >>>>
> >>>> I've also found another "day 1" problem in StubQueue::next():
> >>>>
> >>>>      Stub* next(Stub* s) const                      { int i =
> >>>> index_of(s) + stub_size(s);
> >>>> -                                                   if (i ==
> >>>> _buffer_limit) i = 0;
> >>>> +                                                   // Only wrap
> >>>> around in the non-contiguous case (see stubss.cpp)
> >>>> +                                                   if (i ==
> >>>> _buffer_limit && _queue_end < _buffer_limit) i = 0;
> >>>>                                                       return (i ==
> >>>> _queue_end) ? NULL : stub_at(i);
> >>>>                                                     }
> >>>>
> >>>> The problem was that the method was not prepared to handle the case
> >>>> where _buffer_limit == _queue_end == _buffer_size which lead to an
> >>>> infinite recursion when iterating over a StubQueue with
> >>>> StubQueue::next() until next() returns NULL (as this was for example
> >>>> done with -XX:+PrintInterpreter). But with the new, trimmed CodeBlob
> >>>> we run into exactly this situation.
> >>>
> >>>
> >>> Okay.
> >>>
> >>>>
> >>>> While doing this last fix I also noticed that "StubQueue::stubs_do()",
> >>>> "StubQueue::queues_do()" and "StubQueue::register_queue()" don't seem
> >>>> to be used anywhere in the open code base (please correct me if I'm
> >>>> wrong). What do you think, maybe we should remove this code in a
> >>>> follow up change if it is really not needed?
> >>>
> >>>
> >>> register_queue() is used in constructor. Other 2 you can remove.
> >>> stub_code_begin() and stub_code_end() are not used too -remove.
> >>> I thought we run on linux with flag which warn about unused code.
> >>>
> >>>>
> >>>> Finally, could you please run the new version through JPRT and sponsor
> >>>> it once jdk10/hs will be opened again?
> >>>
> >>>
> >>> Will do when jdk10 "consolidation" is finished. Please, remind me
> later if
> >>> I forget.
> >>>
> >>> Thanks,
> >>> Vladimir
> >>>
> >>>>
> >>>> Thanks,
> >>>> Volker
> >>>>
> >>>>> Thanks,
> >>>>> Vladimir
> >>>>>
> >>>>>
> >>>>> On 9/1/17 8:46 AM, Volker Simonis wrote:
> >>>>>>
> >>>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>> I've decided to split the fix for the 'CodeHeap::contains_blob()'
> >>>>>> problem into its own issue "8187091: ReturnBlobToWrongHeapTest fails
> >>>>>> because of problems in CodeHeap::contains_blob()"
> >>>>>> (https://bugs.openjdk.java.net/browse/JDK-8187091) and started a
> new
> >>>>>> review thread for discussing it at:
> >>>>>>
> >>>>>>
> >>>>>>
> http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-September/028206.html
> >>>>>>
> >>>>>> So please lets keep this thread for discussing the interpreter code
> >>>>>> size issue only. I've prepared a new version of the webrev which is
> >>>>>> the same as the first one with the only difference that the change
> to
> >>>>>> 'CodeHeap::contains_blob()' has been removed:
> >>>>>>
> >>>>>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317.v1/
> >>>>>>
> >>>>>> Thanks,
> >>>>>> Volker
> >>>>>>
> >>>>>>
> >>>>>> On Thu, Aug 31, 2017 at 6:35 PM, Volker Simonis
> >>>>>> <[hidden email]> wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>> On Thu, Aug 31, 2017 at 6:05 PM, Vladimir Kozlov
> >>>>>>> <[hidden email]> wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Very good change. Thank you, Volker.
> >>>>>>>>
> >>>>>>>> About contains_blob(). The problem is that AOTCompiledMethod
> >>>>>>>> allocated
> >>>>>>>> in
> >>>>>>>> CHeap and not in aot code section (which is RO):
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> http://hg.openjdk.java.net/jdk10/hs/hotspot/file/8acd232fb52a/src/share/vm/aot/aotCompiledMethod.hpp#l124
> >>>>>>>>
> >>>>>>>> It is allocated in CHeap after AOT library is loaded. Its
> >>>>>>>> code_begin()
> >>>>>>>> points to AOT code section but AOTCompiledMethod* points outside
> it
> >>>>>>>> (to
> >>>>>>>> normal malloced space) so you can't use (char*)blob address.
> >>>>>>>>
> >>>>>>>
> >>>>>>> Thanks for the explanation - now I got it.
> >>>>>>>
> >>>>>>>> There are 2 ways to fix it, I think.
> >>>>>>>> One is to add new field to CodeBlobLayout and set it to blob*
> address
> >>>>>>>> for
> >>>>>>>> normal CodeCache blobs and to code_begin for AOT code.
> >>>>>>>> Second is to use contains(blob->code_end() - 1) assuming that AOT
> >>>>>>>> code
> >>>>>>>> is
> >>>>>>>> never zero.
> >>>>>>>>
> >>>>>>>
> >>>>>>> I'll give it a try tomorrow and will send out a new webrev.
> >>>>>>>
> >>>>>>> Regards,
> >>>>>>> Volker
> >>>>>>>
> >>>>>>>> Thanks,
> >>>>>>>> Vladimir
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 8/31/17 5:43 AM, Volker Simonis wrote:
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> On Thu, Aug 31, 2017 at 12:14 PM, Claes Redestad
> >>>>>>>>> <[hidden email]> wrote:
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> On 2017-08-31 08:54, Volker Simonis wrote:
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> While working on this, I found another problem which is
> related to
> >>>>>>>>>>> the
> >>>>>>>>>>> fix of JDK-8183573 and leads to crashes when executing the
> JTreg
> >>>>>>>>>>> test
> >>>>>>>>>>> compiler/codecache/stress/ReturnBlobToWrongHeapTest.java.
> >>>>>>>>>>>
> >>>>>>>>>>> The problem is that JDK-8183573 replaced
> >>>>>>>>>>>
> >>>>>>>>>>>        virtual bool contains_blob(const CodeBlob* blob) const {
> >>>>>>>>>>> return
> >>>>>>>>>>> low_boundary() <= (char*) blob && (char*) blob < high(); }
> >>>>>>>>>>>
> >>>>>>>>>>> by:
> >>>>>>>>>>>
> >>>>>>>>>>>        bool contains_blob(const CodeBlob* blob) const { return
> >>>>>>>>>>> contains(blob->code_begin()); }
> >>>>>>>>>>>
> >>>>>>>>>>> But that my be wrong in the corner case where the size of the
> >>>>>>>>>>> CodeBlob's payload is zero (i.e. the CodeBlob consists only of
> the
> >>>>>>>>>>> 'header' - i.e. the C++ object itself) because in that case
> >>>>>>>>>>> CodeBlob::code_begin() points right behind the CodeBlob's
> header
> >>>>>>>>>>> which
> >>>>>>>>>>> is a memory location which doesn't belong to the CodeBlob
> anymore.
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> I recall this change was somehow necessary to allow merging
> >>>>>>>>>> AOTCodeHeap::contains_blob and CodeHead::contains_blob into
> >>>>>>>>>> one devirtualized method, so you need to ensure all AOT tests
> >>>>>>>>>> pass with this change (on linux-x64).
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> All of hotspot/test/aot and hotspot/test/jvmci executed and
> passed
> >>>>>>>>> successful. Are there any other tests I should check?
> >>>>>>>>>
> >>>>>>>>> That said, it is a little hard to follow the stages of your
> change.
> >>>>>>>>> It
> >>>>>>>>> seems like
> >>>>>>>>>
> http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.00/
> >>>>>>>>> was reviewed [1] but then finally the slightly changed version
> from
> >>>>>>>>>
> http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.01/
> >>>>>>>>> was
> >>>>>>>>> checked in and linked to the bug report.
> >>>>>>>>>
> >>>>>>>>> The first, reviewed version of the change still had a correct
> >>>>>>>>> version
> >>>>>>>>> of 'CodeHeap::contains_blob(const CodeBlob* blob)' while the
> second,
> >>>>>>>>> checked in version has the faulty version of that method.
> >>>>>>>>>
> >>>>>>>>> I don't know why you finally did that change to 'contains_blob()'
> >>>>>>>>> but
> >>>>>>>>> I don't see any reason why we shouldn't be able to directly use
> the
> >>>>>>>>> blob's address for inclusion checking. From what I understand, it
> >>>>>>>>> should ALWAYS be contained in the corresponding CodeHeap so no
> >>>>>>>>> reason
> >>>>>>>>> to mess with 'CodeBlob::code_begin()'.
> >>>>>>>>>
> >>>>>>>>> Please let me know if I'm missing something.
> >>>>>>>>>
> >>>>>>>>> [1]
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2017-July/026624.html
> >>>>>>>>>
> >>>>>>>>>> I can't help to wonder if we'd not be better served by
> disallowing
> >>>>>>>>>> zero-sized payloads. Is this something that can ever actually
> >>>>>>>>>> happen except by abuse of the white box API?
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> The corresponding test (ReturnBlobToWrongHeapTest.java)
> specifically
> >>>>>>>>> wants to allocate "segment sized" blocks which is most easily
> >>>>>>>>> achieved
> >>>>>>>>> by allocation zero-sized CodeBlobs. And I think there's nothing
> >>>>>>>>> wrong
> >>>>>>>>> about it if we handle the inclusion tests correctly.
> >>>>>>>>>
> >>>>>>>>> Thank you and best regards,
> >>>>>>>>> Volker
> >>>>>>>>>
> >>>>>>>>>> /Claes
> >>
> >>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8166317: InterpreterCodeSize should be computed

coleen.phillimore

I can sponsor this for you once you rebase, and fix these compilation
errors.
Thanks,
Coleen

On 9/30/17 12:28 AM, Volker Simonis wrote:

> Hi Vladimir,
>
> thanks a lot for remembering these changes!
>
> Regards,
> Volker
>
>
> Vladimir Kozlov <[hidden email]
> <mailto:[hidden email]>> schrieb am Fr. 29. Sep. 2017 um
> 15:47:
>
>     I hit build failure when tried to push changes:
>
>     src/hotspot/share/code/codeBlob.hpp(162) : warning C4267: '=' :
>     conversion from 'size_t' to 'int', possible loss of data
>     src/hotspot/share/code/codeBlob.hpp(163) : warning C4267: '=' :
>     conversion from 'size_t' to 'int', possible loss of data
>
>     I am going to fix it by casting (int):
>
>     +  void adjust_size(size_t used) {
>     +    _size = (int)used;
>     +    _data_offset = (int)used;
>     +    _code_end = (address)this + used;
>     +    _data_end = (address)this + used;
>     +  }
>
>     Note, CodeCache size can't more than 2Gb (max_int) so such casting
>     is fine.
>
>     Vladimir
>
>     On 9/6/17 6:20 AM, Volker Simonis wrote:
>     > On Tue, Sep 5, 2017 at 9:36 PM,  <[hidden email]
>     <mailto:[hidden email]>> wrote:
>     >>
>     >> I was going to make the same comment about the friend
>     declaration in v1, so
>     >> v2 looks better to me.  Looks good.  Thank you for finding a
>     solution to
>     >> this problem that we've had for a long time.  I will sponsor
>     this (remind me
>     >> if I forget after the 18th).
>     >>
>     >
>     > Thanks Coleen! I've updated
>     >
>     > http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317.v2/
>     <http://cr.openjdk.java.net/%7Esimonis/webrevs/2017/8166317.v2/>
>     >
>     > in-place and added you as a second reviewer.
>     >
>     > Regards,
>     > Volker
>     >
>     >
>     >> thanks,
>     >> Coleen
>     >>
>     >>
>     >>
>     >> On 9/5/17 1:17 PM, Vladimir Kozlov wrote:
>     >>>
>     >>> On 9/5/17 9:49 AM, Volker Simonis wrote:
>     >>>>
>     >>>> On Fri, Sep 1, 2017 at 6:16 PM, Vladimir Kozlov
>     >>>> <[hidden email]
>     <mailto:[hidden email]>> wrote:
>     >>>>>
>     >>>>> May be add new CodeBlob's method to adjust sizes instead of
>     directly
>     >>>>> setting
>     >>>>> them in  CodeCache::free_unused_tail(). Then you would not
>     need friend
>     >>>>> class
>     >>>>> CodeCache in CodeBlob.
>     >>>>>
>     >>>>
>     >>>> Changed as suggested (I didn't liked the friend declaration
>     as well :)
>     >>>>
>     >>>>> Also I think adjustment to header_size should be done in
>     >>>>> CodeCache::free_unused_tail() to limit scope of code who
>     knows about
>     >>>>> blob
>     >>>>> layout.
>     >>>>>
>     >>>>
>     >>>> Yes, that's much cleaner. Please find the updated webrev here:
>     >>>>
>     >>>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317.v2/
>     <http://cr.openjdk.java.net/%7Esimonis/webrevs/2017/8166317.v2/>
>     >>>
>     >>>
>     >>> Good.
>     >>>
>     >>>>
>     >>>> I've also found another "day 1" problem in StubQueue::next():
>     >>>>
>     >>>>      Stub* next(Stub* s) const         { int i =
>     >>>> index_of(s) + stub_size(s);
>     >>>> -          if (i ==
>     >>>> _buffer_limit) i = 0;
>     >>>> +          // Only wrap
>     >>>> around in the non-contiguous case (see stubss.cpp)
>     >>>> +          if (i ==
>     >>>> _buffer_limit && _queue_end < _buffer_limit) i = 0;
>     >>>>            return (i ==
>     >>>> _queue_end) ? NULL : stub_at(i);
>     >>>>          }
>     >>>>
>     >>>> The problem was that the method was not prepared to handle
>     the case
>     >>>> where _buffer_limit == _queue_end == _buffer_size which lead
>     to an
>     >>>> infinite recursion when iterating over a StubQueue with
>     >>>> StubQueue::next() until next() returns NULL (as this was for
>     example
>     >>>> done with -XX:+PrintInterpreter). But with the new, trimmed
>     CodeBlob
>     >>>> we run into exactly this situation.
>     >>>
>     >>>
>     >>> Okay.
>     >>>
>     >>>>
>     >>>> While doing this last fix I also noticed that
>     "StubQueue::stubs_do()",
>     >>>> "StubQueue::queues_do()" and "StubQueue::register_queue()"
>     don't seem
>     >>>> to be used anywhere in the open code base (please correct me
>     if I'm
>     >>>> wrong). What do you think, maybe we should remove this code in a
>     >>>> follow up change if it is really not needed?
>     >>>
>     >>>
>     >>> register_queue() is used in constructor. Other 2 you can remove.
>     >>> stub_code_begin() and stub_code_end() are not used too -remove.
>     >>> I thought we run on linux with flag which warn about unused code.
>     >>>
>     >>>>
>     >>>> Finally, could you please run the new version through JPRT
>     and sponsor
>     >>>> it once jdk10/hs will be opened again?
>     >>>
>     >>>
>     >>> Will do when jdk10 "consolidation" is finished. Please, remind
>     me later if
>     >>> I forget.
>     >>>
>     >>> Thanks,
>     >>> Vladimir
>     >>>
>     >>>>
>     >>>> Thanks,
>     >>>> Volker
>     >>>>
>     >>>>> Thanks,
>     >>>>> Vladimir
>     >>>>>
>     >>>>>
>     >>>>> On 9/1/17 8:46 AM, Volker Simonis wrote:
>     >>>>>>
>     >>>>>>
>     >>>>>> Hi,
>     >>>>>>
>     >>>>>> I've decided to split the fix for the
>     'CodeHeap::contains_blob()'
>     >>>>>> problem into its own issue "8187091:
>     ReturnBlobToWrongHeapTest fails
>     >>>>>> because of problems in CodeHeap::contains_blob()"
>     >>>>>> (https://bugs.openjdk.java.net/browse/JDK-8187091) and
>     started a new
>     >>>>>> review thread for discussing it at:
>     >>>>>>
>     >>>>>>
>     >>>>>>
>     http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-September/028206.html
>     >>>>>>
>     >>>>>> So please lets keep this thread for discussing the
>     interpreter code
>     >>>>>> size issue only. I've prepared a new version of the webrev
>     which is
>     >>>>>> the same as the first one with the only difference that the
>     change to
>     >>>>>> 'CodeHeap::contains_blob()' has been removed:
>     >>>>>>
>     >>>>>>
>     http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317.v1/
>     <http://cr.openjdk.java.net/%7Esimonis/webrevs/2017/8166317.v1/>
>     >>>>>>
>     >>>>>> Thanks,
>     >>>>>> Volker
>     >>>>>>
>     >>>>>>
>     >>>>>> On Thu, Aug 31, 2017 at 6:35 PM, Volker Simonis
>     >>>>>> <[hidden email]
>     <mailto:[hidden email]>> wrote:
>     >>>>>>>
>     >>>>>>>
>     >>>>>>> On Thu, Aug 31, 2017 at 6:05 PM, Vladimir Kozlov
>     >>>>>>> <[hidden email]
>     <mailto:[hidden email]>> wrote:
>     >>>>>>>>
>     >>>>>>>>
>     >>>>>>>> Very good change. Thank you, Volker.
>     >>>>>>>>
>     >>>>>>>> About contains_blob(). The problem is that AOTCompiledMethod
>     >>>>>>>> allocated
>     >>>>>>>> in
>     >>>>>>>> CHeap and not in aot code section (which is RO):
>     >>>>>>>>
>     >>>>>>>>
>     >>>>>>>>
>     >>>>>>>>
>     http://hg.openjdk.java.net/jdk10/hs/hotspot/file/8acd232fb52a/src/share/vm/aot/aotCompiledMethod.hpp#l124
>     >>>>>>>>
>     >>>>>>>> It is allocated in CHeap after AOT library is loaded. Its
>     >>>>>>>> code_begin()
>     >>>>>>>> points to AOT code section but AOTCompiledMethod* points
>     outside it
>     >>>>>>>> (to
>     >>>>>>>> normal malloced space) so you can't use (char*)blob address.
>     >>>>>>>>
>     >>>>>>>
>     >>>>>>> Thanks for the explanation - now I got it.
>     >>>>>>>
>     >>>>>>>> There are 2 ways to fix it, I think.
>     >>>>>>>> One is to add new field to CodeBlobLayout and set it to
>     blob* address
>     >>>>>>>> for
>     >>>>>>>> normal CodeCache blobs and to code_begin for AOT code.
>     >>>>>>>> Second is to use contains(blob->code_end() - 1) assuming
>     that AOT
>     >>>>>>>> code
>     >>>>>>>> is
>     >>>>>>>> never zero.
>     >>>>>>>>
>     >>>>>>>
>     >>>>>>> I'll give it a try tomorrow and will send out a new webrev.
>     >>>>>>>
>     >>>>>>> Regards,
>     >>>>>>> Volker
>     >>>>>>>
>     >>>>>>>> Thanks,
>     >>>>>>>> Vladimir
>     >>>>>>>>
>     >>>>>>>>
>     >>>>>>>> On 8/31/17 5:43 AM, Volker Simonis wrote:
>     >>>>>>>>>
>     >>>>>>>>>
>     >>>>>>>>>
>     >>>>>>>>> On Thu, Aug 31, 2017 at 12:14 PM, Claes Redestad
>     >>>>>>>>> <[hidden email]
>     <mailto:[hidden email]>> wrote:
>     >>>>>>>>>>
>     >>>>>>>>>>
>     >>>>>>>>>>
>     >>>>>>>>>>
>     >>>>>>>>>>
>     >>>>>>>>>> On 2017-08-31 08:54, Volker Simonis wrote:
>     >>>>>>>>>>>
>     >>>>>>>>>>>
>     >>>>>>>>>>>
>     >>>>>>>>>>>
>     >>>>>>>>>>> While working on this, I found another problem which
>     is related to
>     >>>>>>>>>>> the
>     >>>>>>>>>>> fix of JDK-8183573 and leads to crashes when executing
>     the JTreg
>     >>>>>>>>>>> test
>     >>>>>>>>>>> compiler/codecache/stress/ReturnBlobToWrongHeapTest.java.
>     >>>>>>>>>>>
>     >>>>>>>>>>> The problem is that JDK-8183573 replaced
>     >>>>>>>>>>>
>     >>>>>>>>>>>        virtual bool contains_blob(const CodeBlob*
>     blob) const {
>     >>>>>>>>>>> return
>     >>>>>>>>>>> low_boundary() <= (char*) blob && (char*) blob < high(); }
>     >>>>>>>>>>>
>     >>>>>>>>>>> by:
>     >>>>>>>>>>>
>     >>>>>>>>>>>        bool contains_blob(const CodeBlob* blob) const
>     { return
>     >>>>>>>>>>> contains(blob->code_begin()); }
>     >>>>>>>>>>>
>     >>>>>>>>>>> But that my be wrong in the corner case where the size
>     of the
>     >>>>>>>>>>> CodeBlob's payload is zero (i.e. the CodeBlob consists
>     only of the
>     >>>>>>>>>>> 'header' - i.e. the C++ object itself) because in that
>     case
>     >>>>>>>>>>> CodeBlob::code_begin() points right behind the
>     CodeBlob's header
>     >>>>>>>>>>> which
>     >>>>>>>>>>> is a memory location which doesn't belong to the
>     CodeBlob anymore.
>     >>>>>>>>>>
>     >>>>>>>>>>
>     >>>>>>>>>>
>     >>>>>>>>>>
>     >>>>>>>>>>
>     >>>>>>>>>> I recall this change was somehow necessary to allow merging
>     >>>>>>>>>> AOTCodeHeap::contains_blob and CodeHead::contains_blob into
>     >>>>>>>>>> one devirtualized method, so you need to ensure all AOT
>     tests
>     >>>>>>>>>> pass with this change (on linux-x64).
>     >>>>>>>>>>
>     >>>>>>>>>
>     >>>>>>>>> All of hotspot/test/aot and hotspot/test/jvmci executed
>     and passed
>     >>>>>>>>> successful. Are there any other tests I should check?
>     >>>>>>>>>
>     >>>>>>>>> That said, it is a little hard to follow the stages of
>     your change.
>     >>>>>>>>> It
>     >>>>>>>>> seems like
>     >>>>>>>>>
>     http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.00/
>     <http://cr.openjdk.java.net/%7Eredestad/scratch/codeheap_contains.00/>
>     >>>>>>>>> was reviewed [1] but then finally the slightly changed
>     version from
>     >>>>>>>>>
>     http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.01/
>     <http://cr.openjdk.java.net/%7Eredestad/scratch/codeheap_contains.01/>
>     >>>>>>>>> was
>     >>>>>>>>> checked in and linked to the bug report.
>     >>>>>>>>>
>     >>>>>>>>> The first, reviewed version of the change still had a
>     correct
>     >>>>>>>>> version
>     >>>>>>>>> of 'CodeHeap::contains_blob(const CodeBlob* blob)' while
>     the second,
>     >>>>>>>>> checked in version has the faulty version of that method.
>     >>>>>>>>>
>     >>>>>>>>> I don't know why you finally did that change to
>     'contains_blob()'
>     >>>>>>>>> but
>     >>>>>>>>> I don't see any reason why we shouldn't be able to
>     directly use the
>     >>>>>>>>> blob's address for inclusion checking. From what I
>     understand, it
>     >>>>>>>>> should ALWAYS be contained in the corresponding CodeHeap
>     so no
>     >>>>>>>>> reason
>     >>>>>>>>> to mess with 'CodeBlob::code_begin()'.
>     >>>>>>>>>
>     >>>>>>>>> Please let me know if I'm missing something.
>     >>>>>>>>>
>     >>>>>>>>> [1]
>     >>>>>>>>>
>     >>>>>>>>>
>     >>>>>>>>>
>     http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2017-July/026624.html
>     >>>>>>>>>
>     >>>>>>>>>> I can't help to wonder if we'd not be better served by
>     disallowing
>     >>>>>>>>>> zero-sized payloads. Is this something that can ever
>     actually
>     >>>>>>>>>> happen except by abuse of the white box API?
>     >>>>>>>>>>
>     >>>>>>>>>
>     >>>>>>>>> The corresponding test (ReturnBlobToWrongHeapTest.java)
>     specifically
>     >>>>>>>>> wants to allocate "segment sized" blocks which is most
>     easily
>     >>>>>>>>> achieved
>     >>>>>>>>> by allocation zero-sized CodeBlobs. And I think there's
>     nothing
>     >>>>>>>>> wrong
>     >>>>>>>>> about it if we handle the inclusion tests correctly.
>     >>>>>>>>>
>     >>>>>>>>> Thank you and best regards,
>     >>>>>>>>> Volker
>     >>>>>>>>>
>     >>>>>>>>>> /Claes
>     >>
>     >>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8166317: InterpreterCodeSize should be computed

Vladimir Kozlov
I rebased it. But there is problem with changes. VM hit guarantee() in this code when run on SPARC in both, fastdebug and product, builds.
Crash happens during build. We can't push this - problem should be investigated and fixed first.

Thanks,
Vladimir

make/Main.gmk:443: recipe for target 'generate-link-opt-data' failed
/usr/ccs/bin/bash: line 4:  9349 Abort                   (core dumped) /s/build/solaris-sparcv9-debug/support/interim-image/bin/java
-XX:DumpLoadedClassList=/s/build/solaris-sparcv9-debug/support/link_opt/classlist -Djava.lang.invoke.MethodHandle.TRACE_RESOLVE=true -cp /s/build/solaris-sparcv9-debug/support/classlist.jar
build.tools.classlist.HelloClasslist 2>&1 > /s/build/solaris-sparcv9-debug/support/link_opt/default_jli_trace.txt
make[3]: *** [/s/build/solaris-sparcv9-debug/support/link_opt/classlist] Error 134
make[2]: *** [generate-link-opt-data] Error 1


# A fatal error has been detected by the Java Runtime Environment:
#
#  Internal Error (/s/open/src/hotspot/share/memory/heap.cpp:233), pid=9349, tid=2
#  guarantee(b == block_at(_next_segment - actual_number_of_segments)) failed: Intermediate allocation!
#
# JRE version:  (10.0) (fastdebug build )
# Java VM: Java HotSpot(TM) 64-Bit Server VM (fastdebug 10-internal+0-2017-09-30-014154.8166317, mixed mode, tiered, compressed oops, g1 gc, solaris-sparc)
# Core dump will be written. Default location: /s/open/make/core or core.9349
#
# If you would like to submit a bug report, please visit:
#   http://bugreport.java.com/bugreport/crash.jsp
#

---------------  S U M M A R Y ------------

Command Line: -XX:DumpLoadedClassList=/s/build/solaris-sparcv9-debug/support/link_opt/classlist -Djava.lang.invoke.MethodHandle.TRACE_RESOLVE=true build.tools.classlist.HelloClasslist

Host: sca00dbv, Sparcv9 64 bit 3600 MHz, 16 cores, 32G, Oracle Solaris 11.2 SPARC
Time: Sat Sep 30 03:29:46 2017 UTC elapsed time: 0 seconds (0d 0h 0m 0s)

---------------  T H R E A D  ---------------

Current thread (0x000000010012f000):  JavaThread "Unknown thread" [_thread_in_vm, id=2, stack(0x0007fffef9700000,0x0007fffef9800000)]

Stack: [0x0007fffef9700000,0x0007fffef9800000],  sp=0x0007fffef97ff020,  free space=1020k
Native frames: (J=compiled Java code, A=aot compiled Java code, j=interpreted, Vv=VM code, C=native code)
V  [libjvm.so+0x1f94508]  void VMError::report_and_die(int,const char*,const char*,void*,Thread*,unsigned char*,void*,void*,const char*,int,unsigned long)+0xa58
V  [libjvm.so+0x1f93a3c]  void VMError::report_and_die(Thread*,const char*,int,const char*,const char*,void*)+0x3c
V  [libjvm.so+0xd02f38]  void report_vm_error(const char*,int,const char*,const char*,...)+0x78
V  [libjvm.so+0xfc219c]  void CodeHeap::deallocate_tail(void*,unsigned long)+0xec
V  [libjvm.so+0xbf4f14]  void CodeCache::free_unused_tail(CodeBlob*,unsigned long)+0xe4
V  [libjvm.so+0x1e0ae70]  void StubQueue::deallocate_unused_tail()+0x40
V  [libjvm.so+0x1e7452c]  void TemplateInterpreter::initialize()+0x19c
V  [libjvm.so+0x1051220]  void interpreter_init()+0x20
V  [libjvm.so+0x10116e0]  int init_globals()+0xf0
V  [libjvm.so+0x1ed8548]  int Threads::create_vm(JavaVMInitArgs*,bool*)+0x4a8
V  [libjvm.so+0x11c7b58]  int JNI_CreateJavaVM_inner(JavaVM_**,void**,void*)+0x108
C  [libjli.so+0x7950]  InitializeJVM+0x100


On 10/2/17 7:55 AM, [hidden email] wrote:

>
> I can sponsor this for you once you rebase, and fix these compilation errors.
> Thanks,
> Coleen
>
> On 9/30/17 12:28 AM, Volker Simonis wrote:
>> Hi Vladimir,
>>
>> thanks a lot for remembering these changes!
>>
>> Regards,
>> Volker
>>
>>
>> Vladimir Kozlov <[hidden email] <mailto:[hidden email]>> schrieb am Fr. 29. Sep. 2017 um 15:47:
>>
>>     I hit build failure when tried to push changes:
>>
>>     src/hotspot/share/code/codeBlob.hpp(162) : warning C4267: '=' : conversion from 'size_t' to 'int', possible loss of data
>>     src/hotspot/share/code/codeBlob.hpp(163) : warning C4267: '=' : conversion from 'size_t' to 'int', possible loss of data
>>
>>     I am going to fix it by casting (int):
>>
>>     +  void adjust_size(size_t used) {
>>     +    _size = (int)used;
>>     +    _data_offset = (int)used;
>>     +    _code_end = (address)this + used;
>>     +    _data_end = (address)this + used;
>>     +  }
>>
>>     Note, CodeCache size can't more than 2Gb (max_int) so such casting is fine.
>>
>>     Vladimir
>>
>>     On 9/6/17 6:20 AM, Volker Simonis wrote:
>>     > On Tue, Sep 5, 2017 at 9:36 PM,  <[hidden email] <mailto:[hidden email]>> wrote:
>>     >>
>>     >> I was going to make the same comment about the friend declaration in v1, so
>>     >> v2 looks better to me.  Looks good.  Thank you for finding a solution to
>>     >> this problem that we've had for a long time.  I will sponsor this (remind me
>>     >> if I forget after the 18th).
>>     >>
>>     >
>>     > Thanks Coleen! I've updated
>>     >
>>     > http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317.v2/ <http://cr.openjdk.java.net/%7Esimonis/webrevs/2017/8166317.v2/>
>>     >
>>     > in-place and added you as a second reviewer.
>>     >
>>     > Regards,
>>     > Volker
>>     >
>>     >
>>     >> thanks,
>>     >> Coleen
>>     >>
>>     >>
>>     >>
>>     >> On 9/5/17 1:17 PM, Vladimir Kozlov wrote:
>>     >>>
>>     >>> On 9/5/17 9:49 AM, Volker Simonis wrote:
>>     >>>>
>>     >>>> On Fri, Sep 1, 2017 at 6:16 PM, Vladimir Kozlov
>>     >>>> <[hidden email] <mailto:[hidden email]>> wrote:
>>     >>>>>
>>     >>>>> May be add new CodeBlob's method to adjust sizes instead of directly
>>     >>>>> setting
>>     >>>>> them in  CodeCache::free_unused_tail(). Then you would not need friend
>>     >>>>> class
>>     >>>>> CodeCache in CodeBlob.
>>     >>>>>
>>     >>>>
>>     >>>> Changed as suggested (I didn't liked the friend declaration as well :)
>>     >>>>
>>     >>>>> Also I think adjustment to header_size should be done in
>>     >>>>> CodeCache::free_unused_tail() to limit scope of code who knows about
>>     >>>>> blob
>>     >>>>> layout.
>>     >>>>>
>>     >>>>
>>     >>>> Yes, that's much cleaner. Please find the updated webrev here:
>>     >>>>
>>     >>>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317.v2/ <http://cr.openjdk.java.net/%7Esimonis/webrevs/2017/8166317.v2/>
>>     >>>
>>     >>>
>>     >>> Good.
>>     >>>
>>     >>>>
>>     >>>> I've also found another "day 1" problem in StubQueue::next():
>>     >>>>
>>     >>>>      Stub* next(Stub* s) const         { int i =
>>     >>>> index_of(s) + stub_size(s);
>>     >>>> -          if (i ==
>>     >>>> _buffer_limit) i = 0;
>>     >>>> +          // Only wrap
>>     >>>> around in the non-contiguous case (see stubss.cpp)
>>     >>>> +          if (i ==
>>     >>>> _buffer_limit && _queue_end < _buffer_limit) i = 0;
>>     >>>>            return (i ==
>>     >>>> _queue_end) ? NULL : stub_at(i);
>>     >>>>          }
>>     >>>>
>>     >>>> The problem was that the method was not prepared to handle the case
>>     >>>> where _buffer_limit == _queue_end == _buffer_size which lead to an
>>     >>>> infinite recursion when iterating over a StubQueue with
>>     >>>> StubQueue::next() until next() returns NULL (as this was for example
>>     >>>> done with -XX:+PrintInterpreter). But with the new, trimmed CodeBlob
>>     >>>> we run into exactly this situation.
>>     >>>
>>     >>>
>>     >>> Okay.
>>     >>>
>>     >>>>
>>     >>>> While doing this last fix I also noticed that "StubQueue::stubs_do()",
>>     >>>> "StubQueue::queues_do()" and "StubQueue::register_queue()" don't seem
>>     >>>> to be used anywhere in the open code base (please correct me if I'm
>>     >>>> wrong). What do you think, maybe we should remove this code in a
>>     >>>> follow up change if it is really not needed?
>>     >>>
>>     >>>
>>     >>> register_queue() is used in constructor. Other 2 you can remove.
>>     >>> stub_code_begin() and stub_code_end() are not used too -remove.
>>     >>> I thought we run on linux with flag which warn about unused code.
>>     >>>
>>     >>>>
>>     >>>> Finally, could you please run the new version through JPRT and sponsor
>>     >>>> it once jdk10/hs will be opened again?
>>     >>>
>>     >>>
>>     >>> Will do when jdk10 "consolidation" is finished. Please, remind me later if
>>     >>> I forget.
>>     >>>
>>     >>> Thanks,
>>     >>> Vladimir
>>     >>>
>>     >>>>
>>     >>>> Thanks,
>>     >>>> Volker
>>     >>>>
>>     >>>>> Thanks,
>>     >>>>> Vladimir
>>     >>>>>
>>     >>>>>
>>     >>>>> On 9/1/17 8:46 AM, Volker Simonis wrote:
>>     >>>>>>
>>     >>>>>>
>>     >>>>>> Hi,
>>     >>>>>>
>>     >>>>>> I've decided to split the fix for the 'CodeHeap::contains_blob()'
>>     >>>>>> problem into its own issue "8187091: ReturnBlobToWrongHeapTest fails
>>     >>>>>> because of problems in CodeHeap::contains_blob()"
>>     >>>>>> (https://bugs.openjdk.java.net/browse/JDK-8187091) and started a new
>>     >>>>>> review thread for discussing it at:
>>     >>>>>>
>>     >>>>>>
>>     >>>>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-September/028206.html
>>     >>>>>>
>>     >>>>>> So please lets keep this thread for discussing the interpreter code
>>     >>>>>> size issue only. I've prepared a new version of the webrev which is
>>     >>>>>> the same as the first one with the only difference that the change to
>>     >>>>>> 'CodeHeap::contains_blob()' has been removed:
>>     >>>>>>
>>     >>>>>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317.v1/ <http://cr.openjdk.java.net/%7Esimonis/webrevs/2017/8166317.v1/>
>>     >>>>>>
>>     >>>>>> Thanks,
>>     >>>>>> Volker
>>     >>>>>>
>>     >>>>>>
>>     >>>>>> On Thu, Aug 31, 2017 at 6:35 PM, Volker Simonis
>>     >>>>>> <[hidden email] <mailto:[hidden email]>> wrote:
>>     >>>>>>>
>>     >>>>>>>
>>     >>>>>>> On Thu, Aug 31, 2017 at 6:05 PM, Vladimir Kozlov
>>     >>>>>>> <[hidden email] <mailto:[hidden email]>> wrote:
>>     >>>>>>>>
>>     >>>>>>>>
>>     >>>>>>>> Very good change. Thank you, Volker.
>>     >>>>>>>>
>>     >>>>>>>> About contains_blob(). The problem is that AOTCompiledMethod
>>     >>>>>>>> allocated
>>     >>>>>>>> in
>>     >>>>>>>> CHeap and not in aot code section (which is RO):
>>     >>>>>>>>
>>     >>>>>>>>
>>     >>>>>>>>
>>     >>>>>>>> http://hg.openjdk.java.net/jdk10/hs/hotspot/file/8acd232fb52a/src/share/vm/aot/aotCompiledMethod.hpp#l124
>>     >>>>>>>>
>>     >>>>>>>> It is allocated in CHeap after AOT library is loaded. Its
>>     >>>>>>>> code_begin()
>>     >>>>>>>> points to AOT code section but AOTCompiledMethod* points outside it
>>     >>>>>>>> (to
>>     >>>>>>>> normal malloced space) so you can't use (char*)blob address.
>>     >>>>>>>>
>>     >>>>>>>
>>     >>>>>>> Thanks for the explanation - now I got it.
>>     >>>>>>>
>>     >>>>>>>> There are 2 ways to fix it, I think.
>>     >>>>>>>> One is to add new field to CodeBlobLayout and set it to blob* address
>>     >>>>>>>> for
>>     >>>>>>>> normal CodeCache blobs and to code_begin for AOT code.
>>     >>>>>>>> Second is to use contains(blob->code_end() - 1) assuming that AOT
>>     >>>>>>>> code
>>     >>>>>>>> is
>>     >>>>>>>> never zero.
>>     >>>>>>>>
>>     >>>>>>>
>>     >>>>>>> I'll give it a try tomorrow and will send out a new webrev.
>>     >>>>>>>
>>     >>>>>>> Regards,
>>     >>>>>>> Volker
>>     >>>>>>>
>>     >>>>>>>> Thanks,
>>     >>>>>>>> Vladimir
>>     >>>>>>>>
>>     >>>>>>>>
>>     >>>>>>>> On 8/31/17 5:43 AM, Volker Simonis wrote:
>>     >>>>>>>>>
>>     >>>>>>>>>
>>     >>>>>>>>>
>>     >>>>>>>>> On Thu, Aug 31, 2017 at 12:14 PM, Claes Redestad
>>     >>>>>>>>> <[hidden email] <mailto:[hidden email]>> wrote:
>>     >>>>>>>>>>
>>     >>>>>>>>>>
>>     >>>>>>>>>>
>>     >>>>>>>>>>
>>     >>>>>>>>>>
>>     >>>>>>>>>> On 2017-08-31 08:54, Volker Simonis wrote:
>>     >>>>>>>>>>>
>>     >>>>>>>>>>>
>>     >>>>>>>>>>>
>>     >>>>>>>>>>>
>>     >>>>>>>>>>> While working on this, I found another problem which is related to
>>     >>>>>>>>>>> the
>>     >>>>>>>>>>> fix of JDK-8183573 and leads to crashes when executing the JTreg
>>     >>>>>>>>>>> test
>>     >>>>>>>>>>> compiler/codecache/stress/ReturnBlobToWrongHeapTest.java.
>>     >>>>>>>>>>>
>>     >>>>>>>>>>> The problem is that JDK-8183573 replaced
>>     >>>>>>>>>>>
>>     >>>>>>>>>>>        virtual bool contains_blob(const CodeBlob* blob) const {
>>     >>>>>>>>>>> return
>>     >>>>>>>>>>> low_boundary() <= (char*) blob && (char*) blob < high(); }
>>     >>>>>>>>>>>
>>     >>>>>>>>>>> by:
>>     >>>>>>>>>>>
>>     >>>>>>>>>>>        bool contains_blob(const CodeBlob* blob) const { return
>>     >>>>>>>>>>> contains(blob->code_begin()); }
>>     >>>>>>>>>>>
>>     >>>>>>>>>>> But that my be wrong in the corner case where the size of the
>>     >>>>>>>>>>> CodeBlob's payload is zero (i.e. the CodeBlob consists only of the
>>     >>>>>>>>>>> 'header' - i.e. the C++ object itself) because in that case
>>     >>>>>>>>>>> CodeBlob::code_begin() points right behind the CodeBlob's header
>>     >>>>>>>>>>> which
>>     >>>>>>>>>>> is a memory location which doesn't belong to the CodeBlob anymore.
>>     >>>>>>>>>>
>>     >>>>>>>>>>
>>     >>>>>>>>>>
>>     >>>>>>>>>>
>>     >>>>>>>>>>
>>     >>>>>>>>>> I recall this change was somehow necessary to allow merging
>>     >>>>>>>>>> AOTCodeHeap::contains_blob and CodeHead::contains_blob into
>>     >>>>>>>>>> one devirtualized method, so you need to ensure all AOT tests
>>     >>>>>>>>>> pass with this change (on linux-x64).
>>     >>>>>>>>>>
>>     >>>>>>>>>
>>     >>>>>>>>> All of hotspot/test/aot and hotspot/test/jvmci executed and passed
>>     >>>>>>>>> successful. Are there any other tests I should check?
>>     >>>>>>>>>
>>     >>>>>>>>> That said, it is a little hard to follow the stages of your change.
>>     >>>>>>>>> It
>>     >>>>>>>>> seems like
>>     >>>>>>>>> http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.00/ <http://cr.openjdk.java.net/%7Eredestad/scratch/codeheap_contains.00/>
>>     >>>>>>>>> was reviewed [1] but then finally the slightly changed version from
>>     >>>>>>>>> http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.01/ <http://cr.openjdk.java.net/%7Eredestad/scratch/codeheap_contains.01/>
>>     >>>>>>>>> was
>>     >>>>>>>>> checked in and linked to the bug report.
>>     >>>>>>>>>
>>     >>>>>>>>> The first, reviewed version of the change still had a correct
>>     >>>>>>>>> version
>>     >>>>>>>>> of 'CodeHeap::contains_blob(const CodeBlob* blob)' while the second,
>>     >>>>>>>>> checked in version has the faulty version of that method.
>>     >>>>>>>>>
>>     >>>>>>>>> I don't know why you finally did that change to 'contains_blob()'
>>     >>>>>>>>> but
>>     >>>>>>>>> I don't see any reason why we shouldn't be able to directly use the
>>     >>>>>>>>> blob's address for inclusion checking. From what I understand, it
>>     >>>>>>>>> should ALWAYS be contained in the corresponding CodeHeap so no
>>     >>>>>>>>> reason
>>     >>>>>>>>> to mess with 'CodeBlob::code_begin()'.
>>     >>>>>>>>>
>>     >>>>>>>>> Please let me know if I'm missing something.
>>     >>>>>>>>>
>>     >>>>>>>>> [1]
>>     >>>>>>>>>
>>     >>>>>>>>>
>>     >>>>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2017-July/026624.html
>>     >>>>>>>>>
>>     >>>>>>>>>> I can't help to wonder if we'd not be better served by disallowing
>>     >>>>>>>>>> zero-sized payloads. Is this something that can ever actually
>>     >>>>>>>>>> happen except by abuse of the white box API?
>>     >>>>>>>>>>
>>     >>>>>>>>>
>>     >>>>>>>>> The corresponding test (ReturnBlobToWrongHeapTest.java) specifically
>>     >>>>>>>>> wants to allocate "segment sized" blocks which is most easily
>>     >>>>>>>>> achieved
>>     >>>>>>>>> by allocation zero-sized CodeBlobs. And I think there's nothing
>>     >>>>>>>>> wrong
>>     >>>>>>>>> about it if we handle the inclusion tests correctly.
>>     >>>>>>>>>
>>     >>>>>>>>> Thank you and best regards,
>>     >>>>>>>>> Volker
>>     >>>>>>>>>
>>     >>>>>>>>>> /Claes
>>     >>
>>     >>
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8166317: InterpreterCodeSize should be computed

Volker Simonis
Thanks Vladimir,

I'll take a look at the problem next week when I'm back from JavaOne.

Regards,
Volker

Vladimir Kozlov <[hidden email]> schrieb am Di. 3. Okt. 2017 um
12:43:

> I rebased it. But there is problem with changes. VM hit guarantee() in
> this code when run on SPARC in both, fastdebug and product, builds.
> Crash happens during build. We can't push this - problem should be
> investigated and fixed first.
>
> Thanks,
> Vladimir
>
> make/Main.gmk:443: recipe for target 'generate-link-opt-data' failed
> /usr/ccs/bin/bash: line 4:  9349 Abort                   (core dumped)
> /s/build/solaris-sparcv9-debug/support/interim-image/bin/java
> -XX:DumpLoadedClassList=/s/build/solaris-sparcv9-debug/support/link_opt/classlist
> -Djava.lang.invoke.MethodHandle.TRACE_RESOLVE=true -cp
> /s/build/solaris-sparcv9-debug/support/classlist.jar
> build.tools.classlist.HelloClasslist 2>&1 >
> /s/build/solaris-sparcv9-debug/support/link_opt/default_jli_trace.txt
> make[3]: *** [/s/build/solaris-sparcv9-debug/support/link_opt/classlist]
> Error 134
> make[2]: *** [generate-link-opt-data] Error 1
>
>
> # A fatal error has been detected by the Java Runtime Environment:
> #
> #  Internal Error (/s/open/src/hotspot/share/memory/heap.cpp:233),
> pid=9349, tid=2
> #  guarantee(b == block_at(_next_segment - actual_number_of_segments))
> failed: Intermediate allocation!
> #
> # JRE version:  (10.0) (fastdebug build )
> # Java VM: Java HotSpot(TM) 64-Bit Server VM (fastdebug
> 10-internal+0-2017-09-30-014154.8166317, mixed mode, tiered, compressed
> oops, g1 gc, solaris-sparc)
> # Core dump will be written. Default location: /s/open/make/core or
> core.9349
> #
> # If you would like to submit a bug report, please visit:
> #   http://bugreport.java.com/bugreport/crash.jsp
> #
>
> ---------------  S U M M A R Y ------------
>
> Command Line:
> -XX:DumpLoadedClassList=/s/build/solaris-sparcv9-debug/support/link_opt/classlist
> -Djava.lang.invoke.MethodHandle.TRACE_RESOLVE=true
> build.tools.classlist.HelloClasslist
>
> Host: sca00dbv, Sparcv9 64 bit 3600 MHz, 16 cores, 32G, Oracle Solaris
> 11.2 SPARC
> Time: Sat Sep 30 03:29:46 2017 UTC elapsed time: 0 seconds (0d 0h 0m 0s)
>
> ---------------  T H R E A D  ---------------
>
> Current thread (0x000000010012f000):  JavaThread "Unknown thread"
> [_thread_in_vm, id=2, stack(0x0007fffef9700000,0x0007fffef9800000)]
>
> Stack: [0x0007fffef9700000,0x0007fffef9800000],  sp=0x0007fffef97ff020,
> free space=1020k
> Native frames: (J=compiled Java code, A=aot compiled Java code,
> j=interpreted, Vv=VM code, C=native code)
> V  [libjvm.so+0x1f94508]  void VMError::report_and_die(int,const
> char*,const char*,void*,Thread*,unsigned char*,void*,void*,const
> char*,int,unsigned long)+0xa58
> V  [libjvm.so+0x1f93a3c]  void VMError::report_and_die(Thread*,const
> char*,int,const char*,const char*,void*)+0x3c
> V  [libjvm.so+0xd02f38]  void report_vm_error(const char*,int,const
> char*,const char*,...)+0x78
> V  [libjvm.so+0xfc219c]  void CodeHeap::deallocate_tail(void*,unsigned
> long)+0xec
> V  [libjvm.so+0xbf4f14]  void
> CodeCache::free_unused_tail(CodeBlob*,unsigned long)+0xe4
> V  [libjvm.so+0x1e0ae70]  void StubQueue::deallocate_unused_tail()+0x40
> V  [libjvm.so+0x1e7452c]  void TemplateInterpreter::initialize()+0x19c
> V  [libjvm.so+0x1051220]  void interpreter_init()+0x20
> V  [libjvm.so+0x10116e0]  int init_globals()+0xf0
> V  [libjvm.so+0x1ed8548]  int
> Threads::create_vm(JavaVMInitArgs*,bool*)+0x4a8
> V  [libjvm.so+0x11c7b58]  int
> JNI_CreateJavaVM_inner(JavaVM_**,void**,void*)+0x108
> C  [libjli.so+0x7950]  InitializeJVM+0x100
>
>
> On 10/2/17 7:55 AM, [hidden email] wrote:
> >
> > I can sponsor this for you once you rebase, and fix these compilation
> errors.
> > Thanks,
> > Coleen
> >
> > On 9/30/17 12:28 AM, Volker Simonis wrote:
> >> Hi Vladimir,
> >>
> >> thanks a lot for remembering these changes!
> >>
> >> Regards,
> >> Volker
> >>
> >>
> >> Vladimir Kozlov <[hidden email] <mailto:
> [hidden email]>> schrieb am Fr. 29. Sep. 2017 um 15:47:
> >>
> >>     I hit build failure when tried to push changes:
> >>
> >>     src/hotspot/share/code/codeBlob.hpp(162) : warning C4267: '=' :
> conversion from 'size_t' to 'int', possible loss of data
> >>     src/hotspot/share/code/codeBlob.hpp(163) : warning C4267: '=' :
> conversion from 'size_t' to 'int', possible loss of data
> >>
> >>     I am going to fix it by casting (int):
> >>
> >>     +  void adjust_size(size_t used) {
> >>     +    _size = (int)used;
> >>     +    _data_offset = (int)used;
> >>     +    _code_end = (address)this + used;
> >>     +    _data_end = (address)this + used;
> >>     +  }
> >>
> >>     Note, CodeCache size can't more than 2Gb (max_int) so such casting
> is fine.
> >>
> >>     Vladimir
> >>
> >>     On 9/6/17 6:20 AM, Volker Simonis wrote:
> >>     > On Tue, Sep 5, 2017 at 9:36 PM,  <[hidden email]
> <mailto:[hidden email]>> wrote:
> >>     >>
> >>     >> I was going to make the same comment about the friend
> declaration in v1, so
> >>     >> v2 looks better to me.  Looks good.  Thank you for finding a
> solution to
> >>     >> this problem that we've had for a long time.  I will sponsor
> this (remind me
> >>     >> if I forget after the 18th).
> >>     >>
> >>     >
> >>     > Thanks Coleen! I've updated
> >>     >
> >>     > http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317.v2/ <
> http://cr.openjdk.java.net/%7Esimonis/webrevs/2017/8166317.v2/>
> >>     >
> >>     > in-place and added you as a second reviewer.
> >>     >
> >>     > Regards,
> >>     > Volker
> >>     >
> >>     >
> >>     >> thanks,
> >>     >> Coleen
> >>     >>
> >>     >>
> >>     >>
> >>     >> On 9/5/17 1:17 PM, Vladimir Kozlov wrote:
> >>     >>>
> >>     >>> On 9/5/17 9:49 AM, Volker Simonis wrote:
> >>     >>>>
> >>     >>>> On Fri, Sep 1, 2017 at 6:16 PM, Vladimir Kozlov
> >>     >>>> <[hidden email] <mailto:[hidden email]>>
> wrote:
> >>     >>>>>
> >>     >>>>> May be add new CodeBlob's method to adjust sizes instead of
> directly
> >>     >>>>> setting
> >>     >>>>> them in  CodeCache::free_unused_tail(). Then you would not
> need friend
> >>     >>>>> class
> >>     >>>>> CodeCache in CodeBlob.
> >>     >>>>>
> >>     >>>>
> >>     >>>> Changed as suggested (I didn't liked the friend declaration as
> well :)
> >>     >>>>
> >>     >>>>> Also I think adjustment to header_size should be done in
> >>     >>>>> CodeCache::free_unused_tail() to limit scope of code who
> knows about
> >>     >>>>> blob
> >>     >>>>> layout.
> >>     >>>>>
> >>     >>>>
> >>     >>>> Yes, that's much cleaner. Please find the updated webrev here:
> >>     >>>>
> >>     >>>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317.v2/ <
> http://cr.openjdk.java.net/%7Esimonis/webrevs/2017/8166317.v2/>
> >>     >>>
> >>     >>>
> >>     >>> Good.
> >>     >>>
> >>     >>>>
> >>     >>>> I've also found another "day 1" problem in StubQueue::next():
> >>     >>>>
> >>     >>>>      Stub* next(Stub* s) const         { int i =
> >>     >>>> index_of(s) + stub_size(s);
> >>     >>>> -          if (i ==
> >>     >>>> _buffer_limit) i = 0;
> >>     >>>> +          // Only wrap
> >>     >>>> around in the non-contiguous case (see stubss.cpp)
> >>     >>>> +          if (i ==
> >>     >>>> _buffer_limit && _queue_end < _buffer_limit) i = 0;
> >>     >>>>            return (i ==
> >>     >>>> _queue_end) ? NULL : stub_at(i);
> >>     >>>>          }
> >>     >>>>
> >>     >>>> The problem was that the method was not prepared to handle the
> case
> >>     >>>> where _buffer_limit == _queue_end == _buffer_size which lead
> to an
> >>     >>>> infinite recursion when iterating over a StubQueue with
> >>     >>>> StubQueue::next() until next() returns NULL (as this was for
> example
> >>     >>>> done with -XX:+PrintInterpreter). But with the new, trimmed
> CodeBlob
> >>     >>>> we run into exactly this situation.
> >>     >>>
> >>     >>>
> >>     >>> Okay.
> >>     >>>
> >>     >>>>
> >>     >>>> While doing this last fix I also noticed that
> "StubQueue::stubs_do()",
> >>     >>>> "StubQueue::queues_do()" and "StubQueue::register_queue()"
> don't seem
> >>     >>>> to be used anywhere in the open code base (please correct me
> if I'm
> >>     >>>> wrong). What do you think, maybe we should remove this code in
> a
> >>     >>>> follow up change if it is really not needed?
> >>     >>>
> >>     >>>
> >>     >>> register_queue() is used in constructor. Other 2 you can remove.
> >>     >>> stub_code_begin() and stub_code_end() are not used too -remove.
> >>     >>> I thought we run on linux with flag which warn about unused
> code.
> >>     >>>
> >>     >>>>
> >>     >>>> Finally, could you please run the new version through JPRT and
> sponsor
> >>     >>>> it once jdk10/hs will be opened again?
> >>     >>>
> >>     >>>
> >>     >>> Will do when jdk10 "consolidation" is finished. Please, remind
> me later if
> >>     >>> I forget.
> >>     >>>
> >>     >>> Thanks,
> >>     >>> Vladimir
> >>     >>>
> >>     >>>>
> >>     >>>> Thanks,
> >>     >>>> Volker
> >>     >>>>
> >>     >>>>> Thanks,
> >>     >>>>> Vladimir
> >>     >>>>>
> >>     >>>>>
> >>     >>>>> On 9/1/17 8:46 AM, Volker Simonis wrote:
> >>     >>>>>>
> >>     >>>>>>
> >>     >>>>>> Hi,
> >>     >>>>>>
> >>     >>>>>> I've decided to split the fix for the
> 'CodeHeap::contains_blob()'
> >>     >>>>>> problem into its own issue "8187091:
> ReturnBlobToWrongHeapTest fails
> >>     >>>>>> because of problems in CodeHeap::contains_blob()"
> >>     >>>>>> (https://bugs.openjdk.java.net/browse/JDK-8187091) and
> started a new
> >>     >>>>>> review thread for discussing it at:
> >>     >>>>>>
> >>     >>>>>>
> >>     >>>>>>
> http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-September/028206.html
> >>     >>>>>>
> >>     >>>>>> So please lets keep this thread for discussing the
> interpreter code
> >>     >>>>>> size issue only. I've prepared a new version of the webrev
> which is
> >>     >>>>>> the same as the first one with the only difference that the
> change to
> >>     >>>>>> 'CodeHeap::contains_blob()' has been removed:
> >>     >>>>>>
> >>     >>>>>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317.v1/
> <http://cr.openjdk.java.net/%7Esimonis/webrevs/2017/8166317.v1/>
> >>     >>>>>>
> >>     >>>>>> Thanks,
> >>     >>>>>> Volker
> >>     >>>>>>
> >>     >>>>>>
> >>     >>>>>> On Thu, Aug 31, 2017 at 6:35 PM, Volker Simonis
> >>     >>>>>> <[hidden email] <mailto:[hidden email]>>
> wrote:
> >>     >>>>>>>
> >>     >>>>>>>
> >>     >>>>>>> On Thu, Aug 31, 2017 at 6:05 PM, Vladimir Kozlov
> >>     >>>>>>> <[hidden email] <mailto:
> [hidden email]>> wrote:
> >>     >>>>>>>>
> >>     >>>>>>>>
> >>     >>>>>>>> Very good change. Thank you, Volker.
> >>     >>>>>>>>
> >>     >>>>>>>> About contains_blob(). The problem is that
> AOTCompiledMethod
> >>     >>>>>>>> allocated
> >>     >>>>>>>> in
> >>     >>>>>>>> CHeap and not in aot code section (which is RO):
> >>     >>>>>>>>
> >>     >>>>>>>>
> >>     >>>>>>>>
> >>     >>>>>>>>
> http://hg.openjdk.java.net/jdk10/hs/hotspot/file/8acd232fb52a/src/share/vm/aot/aotCompiledMethod.hpp#l124
> >>     >>>>>>>>
> >>     >>>>>>>> It is allocated in CHeap after AOT library is loaded. Its
> >>     >>>>>>>> code_begin()
> >>     >>>>>>>> points to AOT code section but AOTCompiledMethod* points
> outside it
> >>     >>>>>>>> (to
> >>     >>>>>>>> normal malloced space) so you can't use (char*)blob
> address.
> >>     >>>>>>>>
> >>     >>>>>>>
> >>     >>>>>>> Thanks for the explanation - now I got it.
> >>     >>>>>>>
> >>     >>>>>>>> There are 2 ways to fix it, I think.
> >>     >>>>>>>> One is to add new field to CodeBlobLayout and set it to
> blob* address
> >>     >>>>>>>> for
> >>     >>>>>>>> normal CodeCache blobs and to code_begin for AOT code.
> >>     >>>>>>>> Second is to use contains(blob->code_end() - 1) assuming
> that AOT
> >>     >>>>>>>> code
> >>     >>>>>>>> is
> >>     >>>>>>>> never zero.
> >>     >>>>>>>>
> >>     >>>>>>>
> >>     >>>>>>> I'll give it a try tomorrow and will send out a new webrev.
> >>     >>>>>>>
> >>     >>>>>>> Regards,
> >>     >>>>>>> Volker
> >>     >>>>>>>
> >>     >>>>>>>> Thanks,
> >>     >>>>>>>> Vladimir
> >>     >>>>>>>>
> >>     >>>>>>>>
> >>     >>>>>>>> On 8/31/17 5:43 AM, Volker Simonis wrote:
> >>     >>>>>>>>>
> >>     >>>>>>>>>
> >>     >>>>>>>>>
> >>     >>>>>>>>> On Thu, Aug 31, 2017 at 12:14 PM, Claes Redestad
> >>     >>>>>>>>> <[hidden email] <mailto:
> [hidden email]>> wrote:
> >>     >>>>>>>>>>
> >>     >>>>>>>>>>
> >>     >>>>>>>>>>
> >>     >>>>>>>>>>
> >>     >>>>>>>>>>
> >>     >>>>>>>>>> On 2017-08-31 08:54, Volker Simonis wrote:
> >>     >>>>>>>>>>>
> >>     >>>>>>>>>>>
> >>     >>>>>>>>>>>
> >>     >>>>>>>>>>>
> >>     >>>>>>>>>>> While working on this, I found another problem which is
> related to
> >>     >>>>>>>>>>> the
> >>     >>>>>>>>>>> fix of JDK-8183573 and leads to crashes when executing
> the JTreg
> >>     >>>>>>>>>>> test
> >>     >>>>>>>>>>>
> compiler/codecache/stress/ReturnBlobToWrongHeapTest.java.
> >>     >>>>>>>>>>>
> >>     >>>>>>>>>>> The problem is that JDK-8183573 replaced
> >>     >>>>>>>>>>>
> >>     >>>>>>>>>>>        virtual bool contains_blob(const CodeBlob* blob)
> const {
> >>     >>>>>>>>>>> return
> >>     >>>>>>>>>>> low_boundary() <= (char*) blob && (char*) blob <
> high(); }
> >>     >>>>>>>>>>>
> >>     >>>>>>>>>>> by:
> >>     >>>>>>>>>>>
> >>     >>>>>>>>>>>        bool contains_blob(const CodeBlob* blob) const {
> return
> >>     >>>>>>>>>>> contains(blob->code_begin()); }
> >>     >>>>>>>>>>>
> >>     >>>>>>>>>>> But that my be wrong in the corner case where the size
> of the
> >>     >>>>>>>>>>> CodeBlob's payload is zero (i.e. the CodeBlob consists
> only of the
> >>     >>>>>>>>>>> 'header' - i.e. the C++ object itself) because in that
> case
> >>     >>>>>>>>>>> CodeBlob::code_begin() points right behind the
> CodeBlob's header
> >>     >>>>>>>>>>> which
> >>     >>>>>>>>>>> is a memory location which doesn't belong to the
> CodeBlob anymore.
> >>     >>>>>>>>>>
> >>     >>>>>>>>>>
> >>     >>>>>>>>>>
> >>     >>>>>>>>>>
> >>     >>>>>>>>>>
> >>     >>>>>>>>>> I recall this change was somehow necessary to allow
> merging
> >>     >>>>>>>>>> AOTCodeHeap::contains_blob and CodeHead::contains_blob
> into
> >>     >>>>>>>>>> one devirtualized method, so you need to ensure all AOT
> tests
> >>     >>>>>>>>>> pass with this change (on linux-x64).
> >>     >>>>>>>>>>
> >>     >>>>>>>>>
> >>     >>>>>>>>> All of hotspot/test/aot and hotspot/test/jvmci executed
> and passed
> >>     >>>>>>>>> successful. Are there any other tests I should check?
> >>     >>>>>>>>>
> >>     >>>>>>>>> That said, it is a little hard to follow the stages of
> your change.
> >>     >>>>>>>>> It
> >>     >>>>>>>>> seems like
> >>     >>>>>>>>>
> http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.00/ <
> http://cr.openjdk.java.net/%7Eredestad/scratch/codeheap_contains.00/>
> >>     >>>>>>>>> was reviewed [1] but then finally the slightly changed
> version from
> >>     >>>>>>>>>
> http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.01/ <
> http://cr.openjdk.java.net/%7Eredestad/scratch/codeheap_contains.01/>
> >>     >>>>>>>>> was
> >>     >>>>>>>>> checked in and linked to the bug report.
> >>     >>>>>>>>>
> >>     >>>>>>>>> The first, reviewed version of the change still had a
> correct
> >>     >>>>>>>>> version
> >>     >>>>>>>>> of 'CodeHeap::contains_blob(const CodeBlob* blob)' while
> the second,
> >>     >>>>>>>>> checked in version has the faulty version of that method.
> >>     >>>>>>>>>
> >>     >>>>>>>>> I don't know why you finally did that change to
> 'contains_blob()'
> >>     >>>>>>>>> but
> >>     >>>>>>>>> I don't see any reason why we shouldn't be able to
> directly use the
> >>     >>>>>>>>> blob's address for inclusion checking. From what I
> understand, it
> >>     >>>>>>>>> should ALWAYS be contained in the corresponding CodeHeap
> so no
> >>     >>>>>>>>> reason
> >>     >>>>>>>>> to mess with 'CodeBlob::code_begin()'.
> >>     >>>>>>>>>
> >>     >>>>>>>>> Please let me know if I'm missing something.
> >>     >>>>>>>>>
> >>     >>>>>>>>> [1]
> >>     >>>>>>>>>
> >>     >>>>>>>>>
> >>     >>>>>>>>>
> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2017-July/026624.html
> >>     >>>>>>>>>
> >>     >>>>>>>>>> I can't help to wonder if we'd not be better served by
> disallowing
> >>     >>>>>>>>>> zero-sized payloads. Is this something that can ever
> actually
> >>     >>>>>>>>>> happen except by abuse of the white box API?
> >>     >>>>>>>>>>
> >>     >>>>>>>>>
> >>     >>>>>>>>> The corresponding test (ReturnBlobToWrongHeapTest.java)
> specifically
> >>     >>>>>>>>> wants to allocate "segment sized" blocks which is most
> easily
> >>     >>>>>>>>> achieved
> >>     >>>>>>>>> by allocation zero-sized CodeBlobs. And I think there's
> nothing
> >>     >>>>>>>>> wrong
> >>     >>>>>>>>> about it if we handle the inclusion tests correctly.
> >>     >>>>>>>>>
> >>     >>>>>>>>> Thank you and best regards,
> >>     >>>>>>>>> Volker
> >>     >>>>>>>>>
> >>     >>>>>>>>>> /Claes
> >>     >>
> >>     >>
> >>
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8166317: InterpreterCodeSize should be computed

Volker Simonis
In reply to this post by Vladimir Kozlov
Hi Vladimir,

I've analyzed the crash. The problem is Sparc specific because on
Sparc we do not call the SharedRuntime for G1 pre/post barriers (i.e.
SharedRuntime::g1_wb_pre() / SharedRuntime::g1_wb_post()) like on
other architectures. Instead we lazily create assembler stubs on the
fly (generate_satb_log_enqueue_if_necessary() /
generate_dirty_card_log_enqueue_if_necessary()) when they are needed.
This happens during the generation of the interpreter and allocates
more memory in the code cache such that we can't shrink the memory
which was initially allocated for the interpreter any more.

Unfortunately we can't easily generate these stubs during
'stubRoutines_init1()' because
'generate_dirty_card_log_enqueue_if_necessary()' needs the byte map
base address which is only initialized in
'CardTableModRefBS::initialize()' during 'univers_init()' which
happens after 'stubRoutines_init1()'.

I'm still thinking about a good way to fix this without too many
platfrom-specific ifdefs.

Regards,
Volker


On Tue, Oct 3, 2017 at 9:46 PM, Vladimir Kozlov
<[hidden email]> wrote:

> I rebased it. But there is problem with changes. VM hit guarantee() in this
> code when run on SPARC in both, fastdebug and product, builds.
> Crash happens during build. We can't push this - problem should be
> investigated and fixed first.
>
> Thanks,
> Vladimir
>
> make/Main.gmk:443: recipe for target 'generate-link-opt-data' failed
> /usr/ccs/bin/bash: line 4:  9349 Abort                   (core dumped)
> /s/build/solaris-sparcv9-debug/support/interim-image/bin/java
> -XX:DumpLoadedClassList=/s/build/solaris-sparcv9-debug/support/link_opt/classlist
> -Djava.lang.invoke.MethodHandle.TRACE_RESOLVE=true -cp
> /s/build/solaris-sparcv9-debug/support/classlist.jar
> build.tools.classlist.HelloClasslist 2>&1 >
> /s/build/solaris-sparcv9-debug/support/link_opt/default_jli_trace.txt
> make[3]: *** [/s/build/solaris-sparcv9-debug/support/link_opt/classlist]
> Error 134
> make[2]: *** [generate-link-opt-data] Error 1
>
>
> # A fatal error has been detected by the Java Runtime Environment:
> #
> #  Internal Error (/s/open/src/hotspot/share/memory/heap.cpp:233), pid=9349,
> tid=2
> #  guarantee(b == block_at(_next_segment - actual_number_of_segments))
> failed: Intermediate allocation!
> #
> # JRE version:  (10.0) (fastdebug build )
> # Java VM: Java HotSpot(TM) 64-Bit Server VM (fastdebug
> 10-internal+0-2017-09-30-014154.8166317, mixed mode, tiered, compressed
> oops, g1 gc, solaris-sparc)
> # Core dump will be written. Default location: /s/open/make/core or
> core.9349
> #
> # If you would like to submit a bug report, please visit:
> #   http://bugreport.java.com/bugreport/crash.jsp
> #
>
> ---------------  S U M M A R Y ------------
>
> Command Line:
> -XX:DumpLoadedClassList=/s/build/solaris-sparcv9-debug/support/link_opt/classlist
> -Djava.lang.invoke.MethodHandle.TRACE_RESOLVE=true
> build.tools.classlist.HelloClasslist
>
> Host: sca00dbv, Sparcv9 64 bit 3600 MHz, 16 cores, 32G, Oracle Solaris 11.2
> SPARC
> Time: Sat Sep 30 03:29:46 2017 UTC elapsed time: 0 seconds (0d 0h 0m 0s)
>
> ---------------  T H R E A D  ---------------
>
> Current thread (0x000000010012f000):  JavaThread "Unknown thread"
> [_thread_in_vm, id=2, stack(0x0007fffef9700000,0x0007fffef9800000)]
>
> Stack: [0x0007fffef9700000,0x0007fffef9800000],  sp=0x0007fffef97ff020,
> free space=1020k
> Native frames: (J=compiled Java code, A=aot compiled Java code,
> j=interpreted, Vv=VM code, C=native code)
> V  [libjvm.so+0x1f94508]  void VMError::report_and_die(int,const char*,const
> char*,void*,Thread*,unsigned char*,void*,void*,const char*,int,unsigned
> long)+0xa58
> V  [libjvm.so+0x1f93a3c]  void VMError::report_and_die(Thread*,const
> char*,int,const char*,const char*,void*)+0x3c
> V  [libjvm.so+0xd02f38]  void report_vm_error(const char*,int,const
> char*,const char*,...)+0x78
> V  [libjvm.so+0xfc219c]  void CodeHeap::deallocate_tail(void*,unsigned
> long)+0xec
> V  [libjvm.so+0xbf4f14]  void CodeCache::free_unused_tail(CodeBlob*,unsigned
> long)+0xe4
> V  [libjvm.so+0x1e0ae70]  void StubQueue::deallocate_unused_tail()+0x40
> V  [libjvm.so+0x1e7452c]  void TemplateInterpreter::initialize()+0x19c
> V  [libjvm.so+0x1051220]  void interpreter_init()+0x20
> V  [libjvm.so+0x10116e0]  int init_globals()+0xf0
> V  [libjvm.so+0x1ed8548]  int
> Threads::create_vm(JavaVMInitArgs*,bool*)+0x4a8
> V  [libjvm.so+0x11c7b58]  int
> JNI_CreateJavaVM_inner(JavaVM_**,void**,void*)+0x108
> C  [libjli.so+0x7950]  InitializeJVM+0x100
>
>
> On 10/2/17 7:55 AM, [hidden email] wrote:
>>
>>
>> I can sponsor this for you once you rebase, and fix these compilation
>> errors.
>> Thanks,
>> Coleen
>>
>> On 9/30/17 12:28 AM, Volker Simonis wrote:
>>>
>>> Hi Vladimir,
>>>
>>> thanks a lot for remembering these changes!
>>>
>>> Regards,
>>> Volker
>>>
>>>
>>> Vladimir Kozlov <[hidden email]
>>> <mailto:[hidden email]>> schrieb am Fr. 29. Sep. 2017 um 15:47:
>>>
>>>     I hit build failure when tried to push changes:
>>>
>>>     src/hotspot/share/code/codeBlob.hpp(162) : warning C4267: '=' :
>>> conversion from 'size_t' to 'int', possible loss of data
>>>     src/hotspot/share/code/codeBlob.hpp(163) : warning C4267: '=' :
>>> conversion from 'size_t' to 'int', possible loss of data
>>>
>>>     I am going to fix it by casting (int):
>>>
>>>     +  void adjust_size(size_t used) {
>>>     +    _size = (int)used;
>>>     +    _data_offset = (int)used;
>>>     +    _code_end = (address)this + used;
>>>     +    _data_end = (address)this + used;
>>>     +  }
>>>
>>>     Note, CodeCache size can't more than 2Gb (max_int) so such casting is
>>> fine.
>>>
>>>     Vladimir
>>>
>>>     On 9/6/17 6:20 AM, Volker Simonis wrote:
>>>     > On Tue, Sep 5, 2017 at 9:36 PM,  <[hidden email]
>>> <mailto:[hidden email]>> wrote:
>>>     >>
>>>     >> I was going to make the same comment about the friend declaration
>>> in v1, so
>>>     >> v2 looks better to me.  Looks good.  Thank you for finding a
>>> solution to
>>>     >> this problem that we've had for a long time.  I will sponsor this
>>> (remind me
>>>     >> if I forget after the 18th).
>>>     >>
>>>     >
>>>     > Thanks Coleen! I've updated
>>>     >
>>>     > http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317.v2/
>>> <http://cr.openjdk.java.net/%7Esimonis/webrevs/2017/8166317.v2/>
>>>     >
>>>     > in-place and added you as a second reviewer.
>>>     >
>>>     > Regards,
>>>     > Volker
>>>     >
>>>     >
>>>     >> thanks,
>>>     >> Coleen
>>>     >>
>>>     >>
>>>     >>
>>>     >> On 9/5/17 1:17 PM, Vladimir Kozlov wrote:
>>>     >>>
>>>     >>> On 9/5/17 9:49 AM, Volker Simonis wrote:
>>>     >>>>
>>>     >>>> On Fri, Sep 1, 2017 at 6:16 PM, Vladimir Kozlov
>>>     >>>> <[hidden email] <mailto:[hidden email]>>
>>> wrote:
>>>     >>>>>
>>>     >>>>> May be add new CodeBlob's method to adjust sizes instead of
>>> directly
>>>     >>>>> setting
>>>     >>>>> them in  CodeCache::free_unused_tail(). Then you would not need
>>> friend
>>>     >>>>> class
>>>     >>>>> CodeCache in CodeBlob.
>>>     >>>>>
>>>     >>>>
>>>     >>>> Changed as suggested (I didn't liked the friend declaration as
>>> well :)
>>>     >>>>
>>>     >>>>> Also I think adjustment to header_size should be done in
>>>     >>>>> CodeCache::free_unused_tail() to limit scope of code who knows
>>> about
>>>     >>>>> blob
>>>     >>>>> layout.
>>>     >>>>>
>>>     >>>>
>>>     >>>> Yes, that's much cleaner. Please find the updated webrev here:
>>>     >>>>
>>>     >>>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317.v2/
>>> <http://cr.openjdk.java.net/%7Esimonis/webrevs/2017/8166317.v2/>
>>>
>>>     >>>
>>>     >>>
>>>     >>> Good.
>>>     >>>
>>>     >>>>
>>>     >>>> I've also found another "day 1" problem in StubQueue::next():
>>>     >>>>
>>>     >>>>      Stub* next(Stub* s) const         { int i =
>>>     >>>> index_of(s) + stub_size(s);
>>>     >>>> -          if (i ==
>>>     >>>> _buffer_limit) i = 0;
>>>     >>>> +          // Only wrap
>>>     >>>> around in the non-contiguous case (see stubss.cpp)
>>>     >>>> +          if (i ==
>>>     >>>> _buffer_limit && _queue_end < _buffer_limit) i = 0;
>>>     >>>>            return (i ==
>>>     >>>> _queue_end) ? NULL : stub_at(i);
>>>     >>>>          }
>>>     >>>>
>>>     >>>> The problem was that the method was not prepared to handle the
>>> case
>>>     >>>> where _buffer_limit == _queue_end == _buffer_size which lead to
>>> an
>>>     >>>> infinite recursion when iterating over a StubQueue with
>>>     >>>> StubQueue::next() until next() returns NULL (as this was for
>>> example
>>>     >>>> done with -XX:+PrintInterpreter). But with the new, trimmed
>>> CodeBlob
>>>     >>>> we run into exactly this situation.
>>>     >>>
>>>     >>>
>>>     >>> Okay.
>>>     >>>
>>>     >>>>
>>>     >>>> While doing this last fix I also noticed that
>>> "StubQueue::stubs_do()",
>>>     >>>> "StubQueue::queues_do()" and "StubQueue::register_queue()" don't
>>> seem
>>>     >>>> to be used anywhere in the open code base (please correct me if
>>> I'm
>>>     >>>> wrong). What do you think, maybe we should remove this code in a
>>>     >>>> follow up change if it is really not needed?
>>>     >>>
>>>     >>>
>>>     >>> register_queue() is used in constructor. Other 2 you can remove.
>>>     >>> stub_code_begin() and stub_code_end() are not used too -remove.
>>>     >>> I thought we run on linux with flag which warn about unused code.
>>>     >>>
>>>     >>>>
>>>     >>>> Finally, could you please run the new version through JPRT and
>>> sponsor
>>>     >>>> it once jdk10/hs will be opened again?
>>>     >>>
>>>     >>>
>>>     >>> Will do when jdk10 "consolidation" is finished. Please, remind me
>>> later if
>>>     >>> I forget.
>>>     >>>
>>>     >>> Thanks,
>>>     >>> Vladimir
>>>     >>>
>>>     >>>>
>>>     >>>> Thanks,
>>>     >>>> Volker
>>>     >>>>
>>>     >>>>> Thanks,
>>>     >>>>> Vladimir
>>>     >>>>>
>>>     >>>>>
>>>     >>>>> On 9/1/17 8:46 AM, Volker Simonis wrote:
>>>     >>>>>>
>>>     >>>>>>
>>>     >>>>>> Hi,
>>>     >>>>>>
>>>     >>>>>> I've decided to split the fix for the
>>> 'CodeHeap::contains_blob()'
>>>     >>>>>> problem into its own issue "8187091: ReturnBlobToWrongHeapTest
>>> fails
>>>     >>>>>> because of problems in CodeHeap::contains_blob()"
>>>     >>>>>> (https://bugs.openjdk.java.net/browse/JDK-8187091) and started
>>> a new
>>>     >>>>>> review thread for discussing it at:
>>>     >>>>>>
>>>     >>>>>>
>>>     >>>>>>
>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-September/028206.html
>>>     >>>>>>
>>>     >>>>>> So please lets keep this thread for discussing the interpreter
>>> code
>>>     >>>>>> size issue only. I've prepared a new version of the webrev
>>> which is
>>>     >>>>>> the same as the first one with the only difference that the
>>> change to
>>>     >>>>>> 'CodeHeap::contains_blob()' has been removed:
>>>     >>>>>>
>>>     >>>>>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8166317.v1/
>>> <http://cr.openjdk.java.net/%7Esimonis/webrevs/2017/8166317.v1/>
>>>     >>>>>>
>>>     >>>>>> Thanks,
>>>     >>>>>> Volker
>>>     >>>>>>
>>>     >>>>>>
>>>     >>>>>> On Thu, Aug 31, 2017 at 6:35 PM, Volker Simonis
>>>     >>>>>> <[hidden email] <mailto:[hidden email]>>
>>> wrote:
>>>     >>>>>>>
>>>     >>>>>>>
>>>     >>>>>>> On Thu, Aug 31, 2017 at 6:05 PM, Vladimir Kozlov
>>>     >>>>>>> <[hidden email]
>>> <mailto:[hidden email]>> wrote:
>>>     >>>>>>>>
>>>     >>>>>>>>
>>>     >>>>>>>> Very good change. Thank you, Volker.
>>>     >>>>>>>>
>>>     >>>>>>>> About contains_blob(). The problem is that AOTCompiledMethod
>>>     >>>>>>>> allocated
>>>     >>>>>>>> in
>>>     >>>>>>>> CHeap and not in aot code section (which is RO):
>>>     >>>>>>>>
>>>     >>>>>>>>
>>>     >>>>>>>>
>>>     >>>>>>>>
>>> http://hg.openjdk.java.net/jdk10/hs/hotspot/file/8acd232fb52a/src/share/vm/aot/aotCompiledMethod.hpp#l124
>>>     >>>>>>>>
>>>     >>>>>>>> It is allocated in CHeap after AOT library is loaded. Its
>>>     >>>>>>>> code_begin()
>>>     >>>>>>>> points to AOT code section but AOTCompiledMethod* points
>>> outside it
>>>     >>>>>>>> (to
>>>     >>>>>>>> normal malloced space) so you can't use (char*)blob address.
>>>     >>>>>>>>
>>>     >>>>>>>
>>>     >>>>>>> Thanks for the explanation - now I got it.
>>>     >>>>>>>
>>>     >>>>>>>> There are 2 ways to fix it, I think.
>>>     >>>>>>>> One is to add new field to CodeBlobLayout and set it to
>>> blob* address
>>>     >>>>>>>> for
>>>     >>>>>>>> normal CodeCache blobs and to code_begin for AOT code.
>>>     >>>>>>>> Second is to use contains(blob->code_end() - 1) assuming
>>> that AOT
>>>     >>>>>>>> code
>>>     >>>>>>>> is
>>>     >>>>>>>> never zero.
>>>     >>>>>>>>
>>>     >>>>>>>
>>>     >>>>>>> I'll give it a try tomorrow and will send out a new webrev.
>>>     >>>>>>>
>>>     >>>>>>> Regards,
>>>     >>>>>>> Volker
>>>     >>>>>>>
>>>     >>>>>>>> Thanks,
>>>     >>>>>>>> Vladimir
>>>     >>>>>>>>
>>>     >>>>>>>>
>>>     >>>>>>>> On 8/31/17 5:43 AM, Volker Simonis wrote:
>>>     >>>>>>>>>
>>>     >>>>>>>>>
>>>     >>>>>>>>>
>>>     >>>>>>>>> On Thu, Aug 31, 2017 at 12:14 PM, Claes Redestad
>>>     >>>>>>>>> <[hidden email]
>>> <mailto:[hidden email]>> wrote:
>>>     >>>>>>>>>>
>>>     >>>>>>>>>>
>>>     >>>>>>>>>>
>>>     >>>>>>>>>>
>>>     >>>>>>>>>>
>>>     >>>>>>>>>> On 2017-08-31 08:54, Volker Simonis wrote:
>>>     >>>>>>>>>>>
>>>     >>>>>>>>>>>
>>>     >>>>>>>>>>>
>>>     >>>>>>>>>>>
>>>     >>>>>>>>>>> While working on this, I found another problem which is
>>> related to
>>>     >>>>>>>>>>> the
>>>     >>>>>>>>>>> fix of JDK-8183573 and leads to crashes when executing
>>> the JTreg
>>>     >>>>>>>>>>> test
>>>     >>>>>>>>>>> compiler/codecache/stress/ReturnBlobToWrongHeapTest.java.
>>>     >>>>>>>>>>>
>>>     >>>>>>>>>>> The problem is that JDK-8183573 replaced
>>>     >>>>>>>>>>>
>>>     >>>>>>>>>>>        virtual bool contains_blob(const CodeBlob* blob)
>>> const {
>>>     >>>>>>>>>>> return
>>>     >>>>>>>>>>> low_boundary() <= (char*) blob && (char*) blob < high();
>>> }
>>>     >>>>>>>>>>>
>>>     >>>>>>>>>>> by:
>>>     >>>>>>>>>>>
>>>     >>>>>>>>>>>        bool contains_blob(const CodeBlob* blob) const {
>>> return
>>>     >>>>>>>>>>> contains(blob->code_begin()); }
>>>     >>>>>>>>>>>
>>>     >>>>>>>>>>> But that my be wrong in the corner case where the size of
>>> the
>>>     >>>>>>>>>>> CodeBlob's payload is zero (i.e. the CodeBlob consists
>>> only of the
>>>     >>>>>>>>>>> 'header' - i.e. the C++ object itself) because in that
>>> case
>>>     >>>>>>>>>>> CodeBlob::code_begin() points right behind the CodeBlob's
>>> header
>>>     >>>>>>>>>>> which
>>>     >>>>>>>>>>> is a memory location which doesn't belong to the CodeBlob
>>> anymore.
>>>     >>>>>>>>>>
>>>     >>>>>>>>>>
>>>     >>>>>>>>>>
>>>     >>>>>>>>>>
>>>     >>>>>>>>>>
>>>     >>>>>>>>>> I recall this change was somehow necessary to allow
>>> merging
>>>     >>>>>>>>>> AOTCodeHeap::contains_blob and CodeHead::contains_blob
>>> into
>>>     >>>>>>>>>> one devirtualized method, so you need to ensure all AOT
>>> tests
>>>     >>>>>>>>>> pass with this change (on linux-x64).
>>>     >>>>>>>>>>
>>>     >>>>>>>>>
>>>     >>>>>>>>> All of hotspot/test/aot and hotspot/test/jvmci executed and
>>> passed
>>>     >>>>>>>>> successful. Are there any other tests I should check?
>>>     >>>>>>>>>
>>>     >>>>>>>>> That said, it is a little hard to follow the stages of your
>>> change.
>>>     >>>>>>>>> It
>>>     >>>>>>>>> seems like
>>>     >>>>>>>>>
>>> http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.00/
>>> <http://cr.openjdk.java.net/%7Eredestad/scratch/codeheap_contains.00/>
>>>     >>>>>>>>> was reviewed [1] but then finally the slightly changed
>>> version from
>>>     >>>>>>>>>
>>> http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.01/
>>> <http://cr.openjdk.java.net/%7Eredestad/scratch/codeheap_contains.01/>
>>>
>>>     >>>>>>>>> was
>>>     >>>>>>>>> checked in and linked to the bug report.
>>>     >>>>>>>>>
>>>     >>>>>>>>> The first, reviewed version of the change still had a
>>> correct
>>>     >>>>>>>>> version
>>>     >>>>>>>>> of 'CodeHeap::contains_blob(const CodeBlob* blob)' while
>>> the second,
>>>     >>>>>>>>> checked in version has the faulty version of that method.
>>>     >>>>>>>>>
>>>     >>>>>>>>> I don't know why you finally did that change to
>>> 'contains_blob()'
>>>     >>>>>>>>> but
>>>     >>>>>>>>> I don't see any reason why we shouldn't be able to directly
>>> use the
>>>     >>>>>>>>> blob's address for inclusion checking. From what I
>>> understand, it
>>>     >>>>>>>>> should ALWAYS be contained in the corresponding CodeHeap so
>>> no
>>>     >>>>>>>>> reason
>>>     >>>>>>>>> to mess with 'CodeBlob::code_begin()'.
>>>     >>>>>>>>>
>>>     >>>>>>>>> Please let me know if I'm missing something.
>>>     >>>>>>>>>
>>>     >>>>>>>>> [1]
>>>     >>>>>>>>>
>>>     >>>>>>>>>
>>>     >>>>>>>>>
>>> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2017-July/026624.html
>>>     >>>>>>>>>
>>>     >>>>>>>>>> I can't help to wonder if we'd not be better served by
>>> disallowing
>>>     >>>>>>>>>> zero-sized payloads. Is this something that can ever
>>> actually
>>>     >>>>>>>>>> happen except by abuse of the white box API?
>>>     >>>>>>>>>>
>>>     >>>>>>>>>
>>>     >>>>>>>>> The corresponding test (ReturnBlobToWrongHeapTest.java)
>>> specifically
>>>     >>>>>>>>> wants to allocate "segment sized" blocks which is most
>>> easily
>>>     >>>>>>>>> achieved
>>>     >>>>>>>>> by allocation zero-sized CodeBlobs. And I think there's
>>> nothing
>>>     >>>>>>>>> wrong
>>>     >>>>>>>>> about it if we handle the inclusion tests correctly.
>>>     >>>>>>>>>
>>>     >>>>>>>>> Thank you and best regards,
>>>     >>>>>>>>> Volker
>>>     >>>>>>>>>
>>>     >>>>>>>>>> /Claes
>>>     >>
>>>     >>
>>>
>>
>
12