RFR(M): 8166317: InterpreterCodeSize should be computed
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Tue Sep 5 19:36:49 UTC 2017
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
>> <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