RFR(M) 8147461: Use byte offsets for vtable start and vtable length offsets
Hi all, As per the previous discussion in mid-December[0] about moving the _vtable_length field to class Klass, here's the first RFR and webrev, according to my suggested plan[1]:
My current plan is to first modify the vtable_length_offset accessor to return a byte offset (which is what it's translated to by all callers).
Then I'll tackle moving the _vtable_len field to Klass.
Finally I'll try to consolidate the vtable related methods to Klass, where they belong.
This change actually consists of three changes: * modifying InstanceKlass::vtable_length_offset to become a byte offset and use the ByteSize type to communicate the scaling. * modifying InstanceKlass::vtable_start_offset to become a byte offset and use the ByteSize type, for symmetry reasons mainly. * adding a vtableEntry::size_in_bytes() since in many places the vtable entry size is used in combination with the vtable start to compute a byte offset for vtable lookups. I don't foresee any issues with the fact that the byte offset is represented as an int, for two reasons: 1) If the offset of any of these grows to over 2 gigabytes then we have a huge footprint problem with InstanceKlass 2) The offsets are converted to byte offsets and stored in ints already in the cpu specific code I've modified. Bug link: https://bugs.openjdk.java.net/browse/JDK-8147461 Webrev: http://cr.openjdk.java.net/~mgerdin/8147461/webrev.0/ Testing: JPRT on Oracle supported platforms, testing on AARCH64 and PPC64 would be much appreciated, appropriate mailing lists have been CC:ed to notify them of the request. [0] http://mail.openjdk.java.net/pipermail/hotspot-dev/2015-December/021152.html [1] http://mail.openjdk.java.net/pipermail/hotspot-dev/2015-December/021224.html Thanks! /Mikael
Hi Mikael, your change builds and runs fine on ppc64. Thanks for considering ppc64 in your patch, Volker On Fri, Jan 15, 2016 at 6:04 PM, Mikael Gerdin <mikael.gerdin@oracle.com> wrote:
Hi all,
As per the previous discussion in mid-December[0] about moving the _vtable_length field to class Klass, here's the first RFR and webrev, according to my suggested plan[1]:
My current plan is to first modify the vtable_length_offset accessor to return a byte offset (which is what it's translated to by all callers).
Then I'll tackle moving the _vtable_len field to Klass.
Finally I'll try to consolidate the vtable related methods to Klass, where they belong.
This change actually consists of three changes: * modifying InstanceKlass::vtable_length_offset to become a byte offset and use the ByteSize type to communicate the scaling. * modifying InstanceKlass::vtable_start_offset to become a byte offset and use the ByteSize type, for symmetry reasons mainly. * adding a vtableEntry::size_in_bytes() since in many places the vtable entry size is used in combination with the vtable start to compute a byte offset for vtable lookups.
I don't foresee any issues with the fact that the byte offset is represented as an int, for two reasons: 1) If the offset of any of these grows to over 2 gigabytes then we have a huge footprint problem with InstanceKlass 2) The offsets are converted to byte offsets and stored in ints already in the cpu specific code I've modified.
Bug link: https://bugs.openjdk.java.net/browse/JDK-8147461 Webrev: http://cr.openjdk.java.net/~mgerdin/8147461/webrev.0/
Testing: JPRT on Oracle supported platforms, testing on AARCH64 and PPC64 would be much appreciated, appropriate mailing lists have been CC:ed to notify them of the request.
[0] http://mail.openjdk.java.net/pipermail/hotspot-dev/2015-December/021152.html [1] http://mail.openjdk.java.net/pipermail/hotspot-dev/2015-December/021224.html
Thanks! /Mikael
Hi Volker, On 2016-01-18 11:53, Volker Simonis wrote:
Hi Mikael,
your change builds and runs fine on ppc64.
Thanks for testing the changes. /Mikael
Thanks for considering ppc64 in your patch, Volker
On Fri, Jan 15, 2016 at 6:04 PM, Mikael Gerdin <mikael.gerdin@oracle.com> wrote:
Hi all,
As per the previous discussion in mid-December[0] about moving the _vtable_length field to class Klass, here's the first RFR and webrev, according to my suggested plan[1]:
My current plan is to first modify the vtable_length_offset accessor to return a byte offset (which is what it's translated to by all callers).
Then I'll tackle moving the _vtable_len field to Klass.
Finally I'll try to consolidate the vtable related methods to Klass, where they belong.
This change actually consists of three changes: * modifying InstanceKlass::vtable_length_offset to become a byte offset and use the ByteSize type to communicate the scaling. * modifying InstanceKlass::vtable_start_offset to become a byte offset and use the ByteSize type, for symmetry reasons mainly. * adding a vtableEntry::size_in_bytes() since in many places the vtable entry size is used in combination with the vtable start to compute a byte offset for vtable lookups.
I don't foresee any issues with the fact that the byte offset is represented as an int, for two reasons: 1) If the offset of any of these grows to over 2 gigabytes then we have a huge footprint problem with InstanceKlass 2) The offsets are converted to byte offsets and stored in ints already in the cpu specific code I've modified.
Bug link: https://bugs.openjdk.java.net/browse/JDK-8147461 Webrev: http://cr.openjdk.java.net/~mgerdin/8147461/webrev.0/
Testing: JPRT on Oracle supported platforms, testing on AARCH64 and PPC64 would be much appreciated, appropriate mailing lists have been CC:ed to notify them of the request.
[0] http://mail.openjdk.java.net/pipermail/hotspot-dev/2015-December/021152.html [1] http://mail.openjdk.java.net/pipermail/hotspot-dev/2015-December/021224.html
Thanks! /Mikael
Hi Mikael, I tried a release and fastdebug build on aarch64. The release build builds fine and passes a basic smoke test but the fastdebug build fails with the following errors. /home/ed/new_jdk9/hs-comp/hotspot/src/share/vm/jvmci/jvmciCompilerToVM.cpp: In static member function 'static void CompilerToVM::Data::initialize()': /home/ed/new_jdk9/hs-comp/hotspot/src/share/vm/jvmci/jvmciCompilerToVM.cpp:153:37: error: cannot convert 'ByteSize' to 'int' in assignment InstanceKlass_vtable_start_offset = InstanceKlass::vtable_start_offset(); ^ /home/ed/new_jdk9/hs-comp/hotspot/src/share/vm/jvmci/jvmciCompilerToVM.cpp:154:38: error: cannot convert 'ByteSize' to 'int' in assignment InstanceKlass_vtable_length_offset = InstanceKlass::vtable_length_offset() * HeapWordSize; ^ After fixing these the fastdebug build completed and passed the same basic tests. All the best, Ed. On Fri, 2016-01-15 at 18:04 +0100, Mikael Gerdin wrote:
Hi all,
As per the previous discussion in mid-December[0] about moving the _vtable_length field to class Klass, here's the first RFR and webrev, according to my suggested plan[1]:
My current plan is to first modify the vtable_length_offset accessor to return a byte offset (which is what it's translated to by all callers).
Then I'll tackle moving the _vtable_len field to Klass.
Finally I'll try to consolidate the vtable related methods to Klass, where they belong.
This change actually consists of three changes: * modifying InstanceKlass::vtable_length_offset to become a byte offset and use the ByteSize type to communicate the scaling. * modifying InstanceKlass::vtable_start_offset to become a byte offset and use the ByteSize type, for symmetry reasons mainly. * adding a vtableEntry::size_in_bytes() since in many places the vtable entry size is used in combination with the vtable start to compute a byte offset for vtable lookups.
I don't foresee any issues with the fact that the byte offset is represented as an int, for two reasons: 1) If the offset of any of these grows to over 2 gigabytes then we have a huge footprint problem with InstanceKlass 2) The offsets are converted to byte offsets and stored in ints already in the cpu specific code I've modified.
Bug link: https://bugs.openjdk.java.net/browse/JDK-8147461 Webrev: http://cr.openjdk.java.net/~mgerdin/8147461/webrev.0/
Testing: JPRT on Oracle supported platforms, testing on AARCH64 and PPC64 would be much appreciated, appropriate mailing lists have been CC:ed to notify them of the request.
[0] http://mail.openjdk.java.net/pipermail/hotspot-dev/2015-December/021152.html [1] http://mail.openjdk.java.net/pipermail/hotspot-dev/2015-December/021224.html
Thanks! /Mikael
Hi Edward, On 2016-01-19 11:07, Edward Nevill wrote:
Hi Mikael,
I tried a release and fastdebug build on aarch64. The release build builds fine and passes a basic smoke test but the fastdebug build fails with the following errors.
/home/ed/new_jdk9/hs-comp/hotspot/src/share/vm/jvmci/jvmciCompilerToVM.cpp: In static member function 'static void CompilerToVM::Data::initialize()': /home/ed/new_jdk9/hs-comp/hotspot/src/share/vm/jvmci/jvmciCompilerToVM.cpp:153:37: error: cannot convert 'ByteSize' to 'int' in assignment InstanceKlass_vtable_start_offset = InstanceKlass::vtable_start_offset(); ^ /home/ed/new_jdk9/hs-comp/hotspot/src/share/vm/jvmci/jvmciCompilerToVM.cpp:154:38: error: cannot convert 'ByteSize' to 'int' in assignment InstanceKlass_vtable_length_offset = InstanceKlass::vtable_length_offset() * HeapWordSize; ^ After fixing these the fastdebug build completed and passed the same basic tests.
Thanks for testing! As to the compilation failures this is probably due to me creating the patch before hs-comp was merged into hs-rt. I'll rebase it and fix the jvmci issue. /Mikael
All the best, Ed.
On Fri, 2016-01-15 at 18:04 +0100, Mikael Gerdin wrote:
Hi all,
As per the previous discussion in mid-December[0] about moving the _vtable_length field to class Klass, here's the first RFR and webrev, according to my suggested plan[1]:
My current plan is to first modify the vtable_length_offset accessor to return a byte offset (which is what it's translated to by all callers).
Then I'll tackle moving the _vtable_len field to Klass.
Finally I'll try to consolidate the vtable related methods to Klass, where they belong.
This change actually consists of three changes: * modifying InstanceKlass::vtable_length_offset to become a byte offset and use the ByteSize type to communicate the scaling. * modifying InstanceKlass::vtable_start_offset to become a byte offset and use the ByteSize type, for symmetry reasons mainly. * adding a vtableEntry::size_in_bytes() since in many places the vtable entry size is used in combination with the vtable start to compute a byte offset for vtable lookups.
I don't foresee any issues with the fact that the byte offset is represented as an int, for two reasons: 1) If the offset of any of these grows to over 2 gigabytes then we have a huge footprint problem with InstanceKlass 2) The offsets are converted to byte offsets and stored in ints already in the cpu specific code I've modified.
Bug link: https://bugs.openjdk.java.net/browse/JDK-8147461 Webrev: http://cr.openjdk.java.net/~mgerdin/8147461/webrev.0/
Testing: JPRT on Oracle supported platforms, testing on AARCH64 and PPC64 would be much appreciated, appropriate mailing lists have been CC:ed to notify them of the request.
[0] http://mail.openjdk.java.net/pipermail/hotspot-dev/2015-December/021152.html [1] http://mail.openjdk.java.net/pipermail/hotspot-dev/2015-December/021224.html
Thanks! /Mikael
Hi again, I've rebased the on hs-rt and had to include some additional changes for JVMCI. I've also updated the copyright years. Unfortunately I can't generate an incremental webrev since i rebased the patch and there's no good way that I know of to make that work with webrev. New webrev at: http://cr.openjdk.java.net/~mgerdin/8147461/webrev.1/ Testing: JPRT again (which includes the JVMCI jtreg tests) /Mikael On 2016-01-15 18:04, Mikael Gerdin wrote:
Hi all,
As per the previous discussion in mid-December[0] about moving the _vtable_length field to class Klass, here's the first RFR and webrev, according to my suggested plan[1]:
My current plan is to first modify the vtable_length_offset accessor to return a byte offset (which is what it's translated to by all callers).
Then I'll tackle moving the _vtable_len field to Klass.
Finally I'll try to consolidate the vtable related methods to Klass, where they belong.
This change actually consists of three changes: * modifying InstanceKlass::vtable_length_offset to become a byte offset and use the ByteSize type to communicate the scaling. * modifying InstanceKlass::vtable_start_offset to become a byte offset and use the ByteSize type, for symmetry reasons mainly. * adding a vtableEntry::size_in_bytes() since in many places the vtable entry size is used in combination with the vtable start to compute a byte offset for vtable lookups.
I don't foresee any issues with the fact that the byte offset is represented as an int, for two reasons: 1) If the offset of any of these grows to over 2 gigabytes then we have a huge footprint problem with InstanceKlass 2) The offsets are converted to byte offsets and stored in ints already in the cpu specific code I've modified.
Bug link: https://bugs.openjdk.java.net/browse/JDK-8147461 Webrev: http://cr.openjdk.java.net/~mgerdin/8147461/webrev.0/
Testing: JPRT on Oracle supported platforms, testing on AARCH64 and PPC64 would be much appreciated, appropriate mailing lists have been CC:ed to notify them of the request.
[0] http://mail.openjdk.java.net/pipermail/hotspot-dev/2015-December/021152.html
[1] http://mail.openjdk.java.net/pipermail/hotspot-dev/2015-December/021224.html
Thanks! /Mikael
Hi Mikael, The changes look good except I think you should get someone from the compiler team to make sure the change in HotSpotResolvedJavaMethodImpl.java and HotSpotVMConfig.java are ok. I'm not sure why you chose to remove instanceKlassVtableStartOffset() rather than just fix it. I think some of your changes may conflict with my changes for JDK-8143608. Coleen is pushing JDK-8143608 for me once hs-rt opens up. I'd appreciate it if you could wait until after then before doing your push. thanks, Chris On 1/20/16 4:31 AM, Mikael Gerdin wrote:
Hi again,
I've rebased the on hs-rt and had to include some additional changes for JVMCI. I've also updated the copyright years. Unfortunately I can't generate an incremental webrev since i rebased the patch and there's no good way that I know of to make that work with webrev.
New webrev at: http://cr.openjdk.java.net/~mgerdin/8147461/webrev.1/
Testing: JPRT again (which includes the JVMCI jtreg tests)
/Mikael
On 2016-01-15 18:04, Mikael Gerdin wrote:
Hi all,
As per the previous discussion in mid-December[0] about moving the _vtable_length field to class Klass, here's the first RFR and webrev, according to my suggested plan[1]:
My current plan is to first modify the vtable_length_offset accessor to return a byte offset (which is what it's translated to by all callers).
Then I'll tackle moving the _vtable_len field to Klass.
Finally I'll try to consolidate the vtable related methods to Klass, where they belong.
This change actually consists of three changes: * modifying InstanceKlass::vtable_length_offset to become a byte offset and use the ByteSize type to communicate the scaling. * modifying InstanceKlass::vtable_start_offset to become a byte offset and use the ByteSize type, for symmetry reasons mainly. * adding a vtableEntry::size_in_bytes() since in many places the vtable entry size is used in combination with the vtable start to compute a byte offset for vtable lookups.
I don't foresee any issues with the fact that the byte offset is represented as an int, for two reasons: 1) If the offset of any of these grows to over 2 gigabytes then we have a huge footprint problem with InstanceKlass 2) The offsets are converted to byte offsets and stored in ints already in the cpu specific code I've modified.
Bug link: https://bugs.openjdk.java.net/browse/JDK-8147461 Webrev: http://cr.openjdk.java.net/~mgerdin/8147461/webrev.0/
Testing: JPRT on Oracle supported platforms, testing on AARCH64 and PPC64 would be much appreciated, appropriate mailing lists have been CC:ed to notify them of the request.
[0] http://mail.openjdk.java.net/pipermail/hotspot-dev/2015-December/021152.html
[1] http://mail.openjdk.java.net/pipermail/hotspot-dev/2015-December/021224.html
Thanks! /Mikael
participants (4)
-
Chris Plummer
-
Edward Nevill
-
Mikael Gerdin
-
Volker Simonis