RFR(M): 8166317: InterpreterCodeSize should be computed
Volker Simonis
volker.simonis at gmail.com
Sat Sep 30 04:28:52 UTC 2017
Hi Vladimir,
thanks a lot for remembering these changes!
Regards,
Volker
Vladimir Kozlov <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> 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/
> >
> > 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> 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