[9] RFR(L) 8166413: Integrate VM part of AOT changes

Thu Nov 3 19:17:14 UTC 2016

Thank you, Coleen

I reverted instanceKlassHandle changes.

Vladimir

On 11/3/16 9:00 AM, Coleen Phillimore wrote:
>
>
> On 11/3/16 7:33 AM, Vladimir Kozlov wrote:
>> Done:
>>
>> java -Xlog:aot+class+load=trace -XX:AOTLibrary=./libhelloworld.so
>> HelloWorld
>> [0.060s][trace][aot,class,load] found  java.lang.Object  in
>> ./libhelloworld.so for classloader 0x7f1fb80eef30 tid=0x00007f1fb801b000
>> ...
>>
>> I updated webrev with your, Coleen, and Stefan all suggestions:
>>
>> http://cr.openjdk.java.net/~kvn/aot/hs.webrev.2/
>>
>> this is delta of changes:
>>
>> http://cr.openjdk.java.net/~kvn/aot/hs.webrev.delta.2/
>
> Thank you for making these changes.  I must have missed these CompiledIC
> => const methodHandle& changes when I went through a while ago.
>
> One minor change though:
>
> http://cr.openjdk.java.net/~kvn/aot/hs.webrev.delta.2/src/share/vm/aot/aotCodeHeap.hpp.udiff.html
>
> http://cr.openjdk.java.net/~kvn/aot/hs.webrev.delta.2/src/share/vm/aot/aotLoader.cpp.udiff.html
>
> http://cr.openjdk.java.net/~kvn/aot/hs.webrev.delta.2/src/share/vm/aot/aotLoader.hpp.udiff.html
>
>
> Since instanceKlassHandle is not a real handle, can you keep that
> passing by value?  I have a script that removes them for jdk 10. Sorry
> for the confusion about these.  The difference is that methodHandle has
> a copy constructor and destructor, so passing by const reference avoids
> copying them.   instanceKlassHandle and KlassHandle are dummy now and
> don't have these so don't add to the code.
>
> I don't need to see another webrev.  Thank you for fixing the logging.
>
> Coleen
>
>
>
>
>
>>
>> Thanks,
>> Vladimir
>>
>> On 11/1/16 3:40 AM, Coleen Phillimore wrote:
>>> 5.  Thanks for pointing out the logging tags Stefan. Yes we would
>>> prefer adding "apt" and "fingerprint" and using the composition of
>>> existing tags for logging.
>>> Thanks
>>> Coleen
>>>
>>>
>>>> On Nov 1, 2016, at 3:38 AM, Stefan Karlsson
>>>> <stefan.karlsson at oracle.com> wrote:
>>>>
>>>> (resending without formatting)
>>>>
>>>> Hi Vladimir,
>>>>
>>>> I just took a quick look at the GC code.
>>>>
>>>> 1) You need to go over the entire patch and fix all the include
>>>> lines that were added. They are are not sorted, as they should.
>>>>
>>>> Some examples:
>>>>
>>>> http://cr.openjdk.java.net/~kvn/aot/hs.webrev/src/share/vm/memory/metaspace.cpp.udiff.html
>>>>
>>>>
>>>>  #include "utilities/debug.hpp"
>>>>  #include "utilities/macros.hpp"
>>>> + #include "aot/aotLoader.hpp"
>>>>
>>>>
>>>> http://cr.openjdk.java.net/~kvn/aot/hs.webrev/src/share/vm/gc/g1/g1RootProcessor.cpp.udiff.html
>>>>
>>>>
>>>>  #include "gc/g1/g1Policy.hpp"
>>>>  #include "gc/g1/g1RootClosures.hpp"
>>>>  #include "gc/g1/g1RootProcessor.hpp"
>>>>  #include "gc/g1/heapRegion.inline.hpp"
>>>>  #include "memory/allocation.inline.hpp"
>>>> + #include "aot/aotLoader.hpp"
>>>>  #include "runtime/fprofiler.hpp"
>>>>  #include "runtime/mutex.hpp"
>>>>  #include "services/management.hpp"
>>>>
>>>> 2) I'd prefer if the the check if AOT is enabled was folded into
>>>> AOTLoader::oops_do, so that the additions to the GC code would be
>>>> less conspicuous.
>>>>
>>>> For example:
>>>> http://cr.openjdk.java.net/~kvn/aot/hs.webrev/src/share/vm/gc/parallel/psMarkSweep.cpp.udiff.html
>>>>
>>>>
>>>>    CodeBlobToOopClosure adjust_from_blobs(adjust_pointer_closure(),
>>>> CodeBlobToOopClosure::FixRelocations);
>>>>    CodeCache::blobs_do(&adjust_from_blobs);
>>>> +   if (UseAOT) {
>>>> +     AOTLoader::oops_do(adjust_pointer_closure());
>>>> +   }
>>>>    StringTable::oops_do(adjust_pointer_closure());
>>>>    ref_processor()->weak_oops_do(adjust_pointer_closure());
>>>> PSScavenge::reference_processor()->weak_oops_do(adjust_pointer_closure());
>>>>
>>>>
>>>> Would be:
>>>>
>>>>    CodeBlobToOopClosure adjust_from_blobs(adjust_pointer_closure(),
>>>> CodeBlobToOopClosure::FixRelocations);
>>>>    CodeCache::blobs_do(&adjust_from_blobs);
>>>> +   AOTLoader::oops_do(adjust_pointer_closure());
>>>>    StringTable::oops_do(adjust_pointer_closure());
>>>>    ref_processor()->weak_oops_do(adjust_pointer_closure());
>>>> PSScavenge::reference_processor()->weak_oops_do(adjust_pointer_closure());
>>>>
>>>>
>>>> 3) aotLoader.hpp uses implements methods using GrowableArray. This
>>>> will expose the growable array functions to all includers of that
>>>> file. Please move all that code out to an aotLoader.inline.hpp file,
>>>> and then remove the unneeded includes from the aotLoader.hpp file.
>>>> 4)
>>>> http://cr.openjdk.java.net/~kvn/aot/hs.webrev/src/share/vm/memory/virtualspace.hpp.udiff.html
>>>>
>>>>
>>>>    // Reserved area
>>>>    char* low_boundary()  const { return _low_boundary; }
>>>>    char* high_boundary() const { return _high_boundary; }
>>>>
>>>> +   void set_low_boundary(char *p)  { _low_boundary = p; }
>>>> +   void set_high_boundary(char *p) { _high_boundary = p; }
>>>> +   void set_low(char *p)           { _low = p; }
>>>> +   void set_high(char *p)          { _high = p; }
>>>> +
>>>>    bool special() const { return _special; }
>>>>
>>>> These seems unsafe to me, but that might be because I don't
>>>> understand how this is used. VirtualSpace has three sections, the
>>>> lower, middle, and the high. The middle section might have another
>>>> alignment (large pages) than the others. Is this property still
>>>> maintained when these functions are used?
>>>>
>>>>
>>>> 5)
>>>> http://cr.openjdk.java.net/~kvn/aot/hs.webrev/src/share/vm/logging/logTag.hpp.udiff.html
>>>>
>>>>
>>>> Did you discuss with the Runtime team about the naming of these
>>>> tags? The other class* tags where split up into multiple tags. For
>>>> example, classload was changed to class,load.
>>>>
>>>> Thanks,
>>>> StefanK
>>>>
>>>>> On 27/10/16 03:15, Vladimir Kozlov wrote:
>>>>> AOT JEP:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8166089
>>>>> Subtask:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8166415
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~kvn/aot/hs.webrev/
>>>>>
>>>>> Please, review Hotspot VM part of AOT changes.
>>>>>
>>>>> Only Linux/x64 platform is supported. 'jaotc' and AOT part of
>>>>> Hotspot will be build only on Linux/x64.
>>>>>
>>>>> AOT code is NOT linked during AOT libraries load as it happens with
>>>>> normal .so libraries. AOT code entry points are not exposed (not
>>>>> global) in AOT libraries. Only class data has global labels which
>>>>> we look for with dlsym(klass_name).
>>>>>
>>>>> AOT-compiled code in AOT libraries is treated by JVM as *extension*
>>>>> of existing CodeCache. When a java class is loaded JVM looks if
>>>>> corresponding AOT-compiled methods exist in loaded AOT libraries
>>>>> and add links to them from java methods descriptors (we have new
>>>>> field Method::_aot_code). AOT-compiled code follows the same
>>>>> invocation/deoptimization/unloading rules as normal JIT-compiled code.
>>>>>
>>>>> Calls in AOT code use the same methods resolution runtime code as
>>>>> calls in JITed code. The difference is call's destination address
>>>>> is loaded indirectly because we can't patch AOT code - it is
>>>>> immutable (to share between multiple JVM instances).
>>>>>
>>>>> Classes and Strings referenced in AOT code are resolved lazily by
>>>>> calling into runtime. All mutable pointers (oops (mostly strings),
>>>>> metadata) are stored and modified in a separate mutable memory (GOT
>>>>> cells) - they are not embedded into AOT code.
>>>>>
>>>>> Changes includes klass fingerprint generation since we need it to
>>>>> find correct klass data in loaded AOT libraries.
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>
>>>>
>>>
>