RFR(M): 8166317: InterpreterCodeSize should be computed
Volker Simonis
volker.simonis at gmail.com
Tue Sep 5 16:49:34 UTC 2017
On Fri, Sep 1, 2017 at 6:16 PM, Vladimir Kozlov
<vladimir.kozlov at oracle.com> 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
>> <volker.simonis at gmail.com> wrote:
>>>
>>> On Thu, Aug 31, 2017 at 6:05 PM, Vladimir Kozlov
>>> <vladimir.kozlov at oracle.com> 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
>>>>> <claes.redestad at oracle.com> 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
More information about the hotspot-dev
mailing list