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
> <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