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

8259925: [Vector API] Unreasonable IndexOutOfBoundsException message when length < vlen #2127

Closed
wants to merge 2 commits into from

Conversation

@DamonFool
Copy link
Member

@DamonFool DamonFool commented Jan 18, 2021

Hi all,

For this reproducer:

import jdk.incubator.vector.ByteVector;
import jdk.incubator.vector.VectorSpecies;

public class Test {
    static final VectorSpecies<Byte> SPECIES_128 = ByteVector.SPECIES_128;
    static byte[] a = new byte[8];
    static byte[] b = new byte[8];

    public static void main(String[] args) {
        ByteVector av = ByteVector.fromArray(SPECIES_128, a, 0);
        av.intoArray(b, 0);
        System.out.println("b: " + b[0]);
    }
}

The following IndexOutOfBoundsException message (length -7) is unreasonable.

Exception in thread "main" java.lang.IndexOutOfBoundsException: Index 0 out of bounds for length -7

It might be better to fix it like this.

Exception in thread "main" java.lang.IndexOutOfBoundsException: Index 0 out of bounds for length 0

Thanks.
Best regards,
Jie


Progress

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

Issue

  • JDK-8259925: [Vector API] Unreasonable IndexOutOfBoundsException message when length < vlen

Download

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

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Jan 18, 2021

/issue add JDK-8259925
/test
/label add core-libs
/cc core-libs

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 18, 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 Jan 18, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 18, 2021

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

@openjdk openjdk bot added the core-libs label Jan 18, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 18, 2021

@DamonFool
The core-libs label was successfully added.

@openjdk
Copy link

@openjdk openjdk bot commented Jan 18, 2021

@DamonFool The core-libs label was already applied.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 18, 2021

Webrevs

@PaulSandoz
Copy link
Member

@PaulSandoz PaulSandoz commented Jan 19, 2021

That change may cause performance issues. I would recommend leaving as is for now even through the error message is not great. Bounds checking is quite sensitive and WIP. Notice that we also have an option to call Objects.checkFromIndexSize which expresses the intent more accurately, but that is currently less optimal (at least it was when i last checked since it is non an intrinsic).

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Jan 20, 2021

Thanks @PaulSandoz for your review and comments.

Updated:

  • The performance issue has been fixed since there is no more operation for common cases.
  • The readability of OOB exception msg has been improved by following the style of Objects.checkFromIndexSize.
  • Less code generated (several blocks of code were optimized out for the Test::test method below).
import jdk.incubator.vector.ByteVector;
import jdk.incubator.vector.VectorSpecies;

public class Test {
    static final VectorSpecies<Byte> SPECIES_128 = ByteVector.SPECIES_128;
    static byte[] a = new byte[88];
    static byte[] b = new byte[88];

    public static void test() {
        ByteVector av = ByteVector.fromArray(SPECIES_128, a, 0);
        av.intoArray(b, 0);
    }

    public static void main(String[] args) {
        for (int i = 0; i < 100000; i++) {
            test();
        }
        System.out.println("b: " + b[0]);
    }
}

What do you think of it?
Thanks.

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Jan 20, 2021

/test

@openjdk
Copy link

@openjdk openjdk bot commented Jan 20, 2021

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

@openjdk openjdk bot added the test-request label Jan 20, 2021
@PaulSandoz
Copy link
Member

@PaulSandoz PaulSandoz commented Jan 20, 2021

Unfortunately it is still problematic because you have replaced the intrinsic check (that performed by Object.checksIndex, or more specifically here).

Further, you have replaced the bounds check options, in place for experimentation. We are not ready yet to collapse our options, further analysis is required as bounds checks can be very sensitive in C2.

I would be open to you adding a third case, so that you can analyze the performance without perturbing the default behavior. I suspect the correct fix is to make intrinsic Objects.checkFromIndexSize in a similar manner to Object.checksIndex.

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Jan 21, 2021

Unfortunately it is still problematic because you have replaced the intrinsic check (that performed by Object.checksIndex, or more specifically here).

Further, you have replaced the bounds check options, in place for experimentation. We are not ready yet to collapse our options, further analysis is required as bounds checks can be very sensitive in C2.

I would be open to you adding a third case, so that you can analyze the performance without perturbing the default behavior. I suspect the correct fix is to make intrinsic Objects.checkFromIndexSize in a similar manner to Object.checksIndex.

Hi @PaulSandoz ,

Thanks for your kind review and comments.

To be honest, I didn't get your first point.
As for the example above, the intrinsified Objects.checkIndex seems to generate more code than inlined if-statements.
So I'm confused why it's a problem to replace the intrinsic check.
Did you mean an intrinsic is always (or for most cases) better than non-intrinc or inlined if-statements?
And why?

Could you please make it more clearer to us?
Thanks.

@PaulSandoz
Copy link
Member

@PaulSandoz PaulSandoz commented Jan 21, 2021

The intrinsic enables C2 to more reliably elide bounds checks. I don't know the exact details but at a high-level it transforms signed values into unsigned values (and therefore the signed comparisons become unsigned comparisons), which helps C2 remove checks when there is a dominating check of, for example, an upper loop bound.

You say "the intrinsified Objects.checkIndex seems to generate more code than inlined if-statements". Can you present some examples of Java code and the corresponding C2 generated assembler where this happens?

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Jan 21, 2021

The intrinsic enables C2 to more reliably elide bounds checks. I don't know the exact details but at a high-level it transforms signed values into unsigned values (and therefore the signed comparisons become unsigned comparisons), which helps C2 remove checks when there is a dominating check of, for example, an upper loop bound.

OK.
Now I can understand what you are worrying about.
Thanks for your clarification.

You say "the intrinsified Objects.checkIndex seems to generate more code than inlined if-statements". Can you present some examples of Java code and the corresponding C2 generated assembler where this happens?

Will do it later.
Thanks.

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Jan 22, 2021

Hi @PaulSandoz ,

I will show you the assembly code next week since it is already Friday night now.

Thanks.

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Jan 26, 2021

The intrinsic enables C2 to more reliably elide bounds checks. I don't know the exact details but at a high-level it transforms signed values into unsigned values (and therefore the signed comparisons become unsigned comparisons), which helps C2 remove checks when there is a dominating check of, for example, an upper loop bound.

You say "the intrinsified Objects.checkIndex seems to generate more code than inlined if-statements". Can you present some examples of Java code and the corresponding C2 generated assembler where this happens?

Hi @PaulSandoz ,

I agree with you that let's keep the code as it is for the sake of performance.

I spent some time looking into the assembly code generated by Objects.checkIndex and inlined if-statements.
Here are the test program [1], running script [2] and diff [3].

  • For testSimple [4] that I checked last week, inlined if-statements [5] is better than Objects.checkIndex [6].
  • However, for testLoop [7], Objects.checkIndex [8] is better than inlined if-statements [9].
    (I'm sorry I didn't check loop methods last week.)
    AFAICS, the inlined if-statements will generate two more instructions [10] to check wether idx >= 0 for each loop iteration.

It seems that the intrinsified Objects.checkIndex will be able to optimize out the lower bound check for loops.
So I also agree with you that an intrinsified method seems the right choice.

Thanks.
Best regards,
Jie

[1] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/Bar.java
[2] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/1.sh
[3] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/1.diff
[4] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/Bar.java#L10
[5] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/3-testSimple.log
[6] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/2-testSimple.log
[7] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/Bar.java#L15
[8] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/2-testLoop.log
[9] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/3-testLoop.log
[10] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/3-testLoop.log#L135

@PaulSandoz
Copy link
Member

@PaulSandoz PaulSandoz commented Jan 26, 2021

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Jan 26, 2021

Hi Jie, Thanks for the detailed analysis. I suspect the difference outside of loops is because of expression "length - (vlen - 1)”. We need to make intrinsic Objects.checkFromIndexSize. Is that something you might be interested in pursuing?

Yes, I'd like to do that in the future.
Thanks.

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 23, 2021

@DamonFool This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@openjdk
Copy link

@openjdk openjdk bot commented Mar 17, 2021

@DamonFool Unknown command test - for a list of valid commands use /help.

@openjdk
Copy link

@openjdk openjdk bot commented Mar 17, 2021

@DamonFool Unknown command test - for a list of valid commands use /help.

@DamonFool DamonFool closed this Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants