RFR(M): 8166317: InterpreterCodeSize should be computed
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Mon Oct 2 14:55:14 UTC 2017
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 <vladimir.kozlov at oracle.com
> <mailto:vladimir.kozlov at oracle.com>> 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, <coleen.phillimore at oracle.com
> <mailto:coleen.phillimore at oracle.com>> 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
> >>>> <vladimir.kozlov at oracle.com
> <mailto: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/
> <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
> >>>>>> <volker.simonis at gmail.com
> <mailto:volker.simonis at gmail.com>> wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>> On Thu, Aug 31, 2017 at 6:05 PM, Vladimir Kozlov
> >>>>>>> <vladimir.kozlov at oracle.com
> <mailto: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
> <mailto: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/
> <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
> >>
> >>
>
More information about the hotspot-dev
mailing list