Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8262096: Vector API fails to work due to VectorShape initialization exception #2722

Closed
wants to merge 9 commits into from

Conversation

DamonFool
Copy link
Member

@DamonFool DamonFool commented Feb 25, 2021

Hi all,

Vector API fails to work when:

  • case 1: MaxVectorSize is set to <=8, or
  • case 2: C2 is disabled

The reason is that {max/preferred} VectorShape initialization fails in both cases.
And the root cause is that VectorSupport_GetMaxLaneCount [1] returns unreasonable values (0 for case 1 and -1 for case 2).

Vector API should not depend on C2 to run.
It should work even there is no JIT compiler since it's a Java-level api.
So let's fix it.

Testing:

  • jdk/incubator/vector with -XX:MaxVectorSize=default/8 on Linux/x64

Thanks.
Best regards,
Jie

[1] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/prims/vectorSupport.cpp#L364


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8262096: Vector API fails to work due to VectorShape initialization exception

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2722/head:pull/2722
$ git checkout pull/2722

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Feb 25, 2021

/issue add JDK-8262096
/test
/label add hotspot-compiler
/label add core-lib
/cc hotspot-compiler
/cc core-lib

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 25, 2021

👋 Welcome back jiefu! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Feb 25, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 25, 2021

@DamonFool This issue is referenced in the PR title - it will now be updated.

@openjdk openjdk bot added the hotspot-compiler label Feb 25, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 25, 2021

@DamonFool
The hotspot-compiler label was successfully added.

@openjdk
Copy link

@openjdk openjdk bot commented Feb 25, 2021

@DamonFool The label core-lib is not a valid label. These labels are valid:

  • serviceability
  • hotspot
  • sound
  • hotspot-compiler
  • kulla
  • i18n
  • shenandoah
  • jdk
  • javadoc
  • 2d
  • security
  • swing
  • hotspot-runtime
  • jmx
  • build
  • nio
  • beans
  • core-libs
  • compiler
  • net
  • hotspot-gc
  • hotspot-jfr
  • awt

@openjdk
Copy link

@openjdk openjdk bot commented Feb 25, 2021

@DamonFool The hotspot-compiler label was already applied.

@openjdk
Copy link

@openjdk openjdk bot commented Feb 25, 2021

@DamonFool The label core-lib is not a valid label. These labels are valid:

  • serviceability
  • hotspot
  • sound
  • hotspot-compiler
  • kulla
  • i18n
  • shenandoah
  • jdk
  • javadoc
  • 2d
  • security
  • swing
  • hotspot-runtime
  • jmx
  • build
  • nio
  • beans
  • core-libs
  • compiler
  • net
  • hotspot-gc
  • hotspot-jfr
  • awt

@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 25, 2021

Webrevs

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Feb 25, 2021

/label add core-libs
/cc core-libs

@openjdk openjdk bot added the core-libs label Feb 25, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 25, 2021

@DamonFool
The core-libs label was successfully added.

@openjdk
Copy link

@openjdk openjdk bot commented Feb 25, 2021

@DamonFool The core-libs label was already applied.

oop mirror = JNIHandles::resolve_non_null(clazz);
if (java_lang_Class::is_primitive(mirror)) {
BasicType bt = java_lang_Class::primitive_type(mirror);
return Matcher::max_vector_size(bt);
}
int min_lane_count = 64 / type2aelembytes(bt);
Copy link
Member

@PaulSandoz PaulSandoz Feb 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am uncertain of the units here. Is the numerator in bits and the denominator in bytes?

Copy link
Member Author

@DamonFool DamonFool Feb 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Paul for your review.

Oops, the numerator should be 8 (bytes).

Updated.
Testing of jdk/incubator/vector (MaxVector=default/8/4 or without C2) is still fine.
Thanks.

Copy link
Member

@PaulSandoz PaulSandoz Feb 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, was the test VectorShapeInitTest passing prior to the fix of the numerator?
Perhaps we should be testing more directly on VectorShape.S_Max_BIT.vectorBitSize() and VectorShape.preferredShape ?
Also, perhaps we can run with C2 disabled, with -XX:TieredStopAtLevel=3 ?

Copy link
Member Author

@DamonFool DamonFool Feb 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, was the test VectorShapeInitTest passing prior to the fix of the numerator?
Yes. It also passed with min_lane_count = 64 / type2aelembytes(bt).

Perhaps we should be testing more directly on VectorShape.S_Max_BIT.vectorBitSize() and VectorShape.preferredShape ?
Ok.
But it that case, I'd like to just re-use jdk/incubator/vector/PreferredSpeciesTest.java.

Also, perhaps we can run with C2 disabled, with -XX:TieredStopAtLevel=3 ?
Fine.
Although this bug wouldn't be triggered with -XX:TieredStopAtLevel=x, it can help test with C1.

Patch had been updated.
Any comments?
Thanks.

Copy link
Member

@PaulSandoz PaulSandoz Feb 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reusing PreferredSpeciesTest is a good idea.

I now realize that C2 needs to be compiled out to trigger some other cases. In that case I think we can remove the execution with -XX:TieredStopAtLevel=x. The test will get executed with various HotSpot configurations by the test infrastructure, eventually...

Looks good.

Copy link
Member Author

@DamonFool DamonFool Feb 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case I think we can remove the execution with -XX:TieredStopAtLevel=x.

Fixed. Thanks.

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Feb 26, 2021

/test

@openjdk
Copy link

@openjdk openjdk bot commented Feb 26, 2021

@DamonFool you need to get approval to run the tests in tier1 for commits up until 724b36d

@openjdk openjdk bot added the test-request label Feb 26, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 26, 2021

@DamonFool This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8262096: Vector API fails to work due to VectorShape initialization exception

Reviewed-by: psandoz, vlivanov

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 29 new commits pushed to the master branch:

  • 96c4321: 8262424: Change multiple get_java_xxx() functions in thread.cpp into one function
  • 0de6abd: 8260966: (fs) Consolidate Linux and macOS implementations of UserDefinedFileAttributeView
  • 5f4bc0a: 8253100: Fix "no comment" warnings in java.base/java.net
  • d185a6c: 8261483: jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java failed with "AssertionError: Should have GCd a method handle by now"
  • 85a5ae8: 8261606: Surprising behavior of step over in String switch
  • be67aaa: 8262726: AArch64: C1 StubAssembler::call_RT can corrupt stack
  • 0f6122b: 8262819: gc/shenandoah/compiler/TestLinkToNativeRBP.java fails with release VMs
  • dd33a8e: 8262461: handle wcstombsdmp return value correctly in unix awt_InputMethod.c
  • 3b350ad: 8261710: SA DSO objects have sizes that are too large
  • fdd1093: 8261552: s390: MacroAssembler::encode_klass_not_null() may produce wrong results for non-zero values of narrow klass base
  • ... and 19 more: https://git.openjdk.java.net/jdk/compare/03d888f463c0a6e3fee70ed8ad606fc0a3082636...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Feb 26, 2021
Copy link

@iwanowww iwanowww left a comment

IMO the fix should be in src/jdk.incubator.vector/share/classes/jdk/incubator/vector/VectorShape.java.

JVM does the right job when it signals vector support is absent (by returning -1).

jdk.incubator.vector implementation should take that into account and choose a preferred shape for pure Java execution mode.

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Feb 26, 2021

IMO the fix should be in src/jdk.incubator.vector/share/classes/jdk/incubator/vector/VectorShape.java.

JVM does the right job when it signals vector support is absent (by returning -1).

jdk.incubator.vector implementation should take that into account and choose a preferred shape for pure Java execution mode.

Hi @iwanowww ,

Thanks for your review.

From the view of C2 compiler, you are right.

But the Java programmer may be confused if we got something like DoubleVector.SPECIES_PREFERRED.length() > VectorSupport.getMaxLaneCount(double.class).
I'd like to keep DoubleVector.SPECIES_PREFERRED.length() <= VectorSupport.getMaxLaneCount(double.class) for Java programmers since the VectorSupport_GetMaxLaneCount is used to implement a Java API.

What do you think?
Thanks.

@iwanowww
Copy link

@iwanowww iwanowww commented Feb 26, 2021

I'd like to keep DoubleVector.SPECIES_PREFERRED.length() <= VectorSupport.getMaxLaneCount(double.class) for Java programmers since the VectorSupport_GetMaxLaneCount is used to implement a Java API.

It doesn't make much sense to me. VectorSupport is an internal API for jdk.incubator.vector to consume.
It's jdk.incubator.vector job to interpret the result and adapt accordingly.

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Feb 27, 2021

I'd like to keep DoubleVector.SPECIES_PREFERRED.length() <= VectorSupport.getMaxLaneCount(double.class) for Java programmers since the VectorSupport_GetMaxLaneCount is used to implement a Java API.

It doesn't make much sense to me. VectorSupport is an internal API for jdk.incubator.vector to consume.
It's jdk.incubator.vector job to interpret the result and adapt accordingly.

Okay, I'm fine to fix it in jdk/incubator/vector/VectorShape.java if we don't keep something like that.

For the updated fix, the {max/preferred} shape will be initialized as shape-64-bit if hotspot doesn't support vectorization.

Testing:

  • jdk/incubator/vector with MaxVectorSize=default/8/4 on Linux/x64
  • jdk/incubator/vector without C2 on Linux/x64

Any comments?
Thanks.

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Feb 27, 2021

/test

@openjdk
Copy link

@openjdk openjdk bot commented Feb 27, 2021

@DamonFool you need to get approval to run the tests in tier1 for commits up until b67b232

Copy link

@iwanowww iwanowww left a comment

For the updated fix, the {max/preferred} shape will be initialized as shape-64-bit if hotspot doesn't support vectorization.

Sounds reasonable.

* @summary Test the initialization of vector shapes
* @modules jdk.incubator.vector java.base/jdk.internal.vm.vector
* @run testng/othervm -XX:MaxVectorSize=8 PreferredSpeciesTest
* @run testng/othervm -XX:MaxVectorSize=4 PreferredSpeciesTest

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-XX:MaxVectorSize is C2-specific. It's better to specify either -XX:-IgnoreUnrecognizedVMOptions or @requires vm.compiler2.enabled.

Copy link
Member Author

@DamonFool DamonFool Feb 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@requires vm.compiler2.enabled had been added.
Thanks.

Copy link
Member Author

@DamonFool DamonFool Feb 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PaulSandoz , are you also OK with the latest version?
Thanks.

Copy link
Member

@PaulSandoz PaulSandoz Mar 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DamonFool I think Vladimir is correct in the layering, in this respect i think we can make things a littler clearer. This seems like a small thing but i think its worth making very explicit as there is some hidden complexity.

What if we add the following method to VectorShape:

    /**
     * Returns the maximum vector bit size for a given element type.
     *
     * @param etype the element type.
     * @return the maximum vector bit.
     */
     /*package-private*/
    static int getMaxVectorBitSize(Class<?> etype) {
        // May return -1 if C2 is not enabled,
        // or a value smaller than the S_64_BIT.vectorBitSize / elementSizeInBits, on say 32-bit platforms
        // If so default to S_64_BIT
        int maxLaneCount = VectorSupport.getMaxLaneCount(etype);
        int elementSizeInBits = LaneType.of(etype).elementSize;
        return Math.max(maxLaneCount * elementSizeInBits, S_64_BIT.vectorBitSize);
    }

It is package private so it can be tested explicitly if need be.

Then we can reuse that method:

    S_Max_BIT(getMaxVectorBitSize(byte.class));
    static VectorShape largestShapeFor(Class<?> etype) {
        return VectorShape.forBitSize(getMaxVectorBitSize(etype));
    }

I think that's correct, but i have not tested. WDYT?

Copy link
Member Author

@DamonFool DamonFool Mar 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DamonFool I think Vladimir is correct in the layering, in this respect i think we can make things a littler clearer. This seems like a small thing but i think its worth making very explicit as there is some hidden complexity.

What if we add the following method to VectorShape:

    /**
     * Returns the maximum vector bit size for a given element type.
     *
     * @param etype the element type.
     * @return the maximum vector bit.
     */
     /*package-private*/
    static int getMaxVectorBitSize(Class<?> etype) {
        // May return -1 if C2 is not enabled,
        // or a value smaller than the S_64_BIT.vectorBitSize / elementSizeInBits, on say 32-bit platforms
        // If so default to S_64_BIT
        int maxLaneCount = VectorSupport.getMaxLaneCount(etype);
        int elementSizeInBits = LaneType.of(etype).elementSize;
        return Math.max(maxLaneCount * elementSizeInBits, S_64_BIT.vectorBitSize);
    }

It is package private so it can be tested explicitly if need be.

Then we can reuse that method:

    S_Max_BIT(getMaxVectorBitSize(byte.class));
    static VectorShape largestShapeFor(Class<?> etype) {
        return VectorShape.forBitSize(getMaxVectorBitSize(etype));
    }

I think that's correct, but i have not tested. WDYT?

Good suggestion.
Updated.

Testing:

  • jdk/incubator/vector with MaxVectorSize=default/8/4 on Linux/x64
  • jdk/incubator/vector without C2 on Linux/x64

Thanks.

@PaulSandoz
Copy link
Member

@PaulSandoz PaulSandoz commented Mar 2, 2021

@DamonFool that worked out better than i expected :-)

One last thing i missed last time. The PreferredSpeciesTest also needs to be executed with no HotSpot flags. No need for another round of review.

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Mar 2, 2021

The PreferredSpeciesTest also needs to be executed with no HotSpot flags.

Yes, we already have this one [1].
So no need to be updated and will push it later.
Thanks.

[1] https://github.com/openjdk/jdk/blob/master/test/jdk/jdk/incubator/vector/PreferredSpeciesTest.java#L33

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Mar 2, 2021

/integrate

@openjdk openjdk bot closed this Mar 2, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Mar 2, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 2, 2021

@DamonFool Since your change was applied there have been 35 commits pushed to the master branch:

  • 93ffe6a: 8262892: minor typo in implSpec comment
  • 4f4d0f5: 8261969: SNIHostName should check if the encoded hostname conform to RFC 3490
  • c92f3bc: 8262876: Shenandoah: Fix comments regarding VM_ShenandoahOperation inheritances
  • 20b9ba5: 8262875: doccheck: empty paragraphs, etc in java.base module
  • f304b74: 8261859: gc/g1/TestStringDeduplicationTableRehash.java failed with "RuntimeException: 'Rehash Count: 0' found in stdout"
  • f18c019: 8247373: ArraysSupport.newLength doc, test, and exception message
  • 96c4321: 8262424: Change multiple get_java_xxx() functions in thread.cpp into one function
  • 0de6abd: 8260966: (fs) Consolidate Linux and macOS implementations of UserDefinedFileAttributeView
  • 5f4bc0a: 8253100: Fix "no comment" warnings in java.base/java.net
  • d185a6c: 8261483: jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java failed with "AssertionError: Should have GCd a method handle by now"
  • ... and 25 more: https://git.openjdk.java.net/jdk/compare/03d888f463c0a6e3fee70ed8ad606fc0a3082636...master

Your commit was automatically rebased without conflicts.

Pushed as commit 40bdf52.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@DamonFool DamonFool deleted the JDK-8262096 branch Mar 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs hotspot-compiler integrated test-request
3 participants