RFR(M): 8166317: InterpreterCodeSize should be computed
Vladimir Kozlov
vladimir.kozlov at oracle.com
Tue Sep 5 17:17:29 UTC 2017
On 9/5/17 9:49 AM, Volker Simonis wrote:
> 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/
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
>>> <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