[SPARK-48021][ML][BUILD] Add `--add-modules=jdk.incubator.vector` to `JavaModuleOptions` #46246

panbingkun · 2024-04-26T12:27:13Z

What changes were proposed in this pull request?

The pr aims to:

add --add-modules=jdk.incubator.vector to JavaModuleOptions
remove jdk.incubator.foreign and -Dforeign.restricted=warn from SparkBuild.scala

Why are the changes needed?

1.jdk.incubator.vector
First introduction: #30810
https://github.com/apache/spark/pull/30810/files#diff-6f545c33f2fcc975200bf208c900a600a593ce6b170180f81e2f93b3efb6cb3e

Why should we add --add-modules=jdk.incubator.vector to JavaModuleOptions,
Because when we only add --add-modules=jdk.incubator.vector to SparkBuild.scala, it will only take effect when compiling, as follows:

build/sbt "mllib-local/Test/runMain org.apache.spark.ml.linalg.BLASBenchmark"
...

However, when we use spark-submit, it is as follows:

./bin/spark-submit --class org.apache.spark.ml.linalg.BLASBenchmark /Users/panbingkun/Developer/spark/spark-community/mllib-local/target/scala-2.13/spark-mllib-local_2.13-4.0.0-SNAPSHOT-tests.jar

Obviously, --add-modules=jdk.incubator.vector does not take effect in the Spark runtime, so I propose adding --add-modules=jdk.incubator.vector to the JavaModuleOptions(Spark runtime options) so that we can improve performance by using hardware-accelerated BLAS operations by default.

After this patch(add --add-modules=jdk.incubator.vector to the JavaModuleOptions), as follows:

2.jdk.incubator.foreign and -Dforeign.restricted=warn
A.First introduction: #32253
https://github.com/apache/spark/pull/32253/files#diff-6f545c33f2fcc975200bf208c900a600a593ce6b170180f81e2f93b3efb6cb3e

Use dev.ludovic.netlib:blas:1.3.2, the class ForeignLinkerBLAS uses jdk.incubator.foreign.* in this version, so we need to add jdk.incubator.foreign and -Dforeign.restricted=warn to SparkBuild.scala
https://github.com/apache/spark/pull/32253/files#diff-9c5fb3d1b7e3b0f54bc5c4182965c4fe1f9023d449017cece3005d3f90e8e4d8

https://github.com/luhenry/netlib/blob/v1.3.2/blas/src/main/java/dev/ludovic/netlib/blas/ForeignLinkerBLAS.java#L36

However, with the iterative development of dev.ludovic.netlib, ForeignLinkerBLAS has experienced one major change, as following:
luhenry/netlib@48e923c

From now on (V3.0.0), jdk.incubator.foreign.* will not be used in dev.ludovic.netlib

Currently, Spark has used the dev.ludovic.netlib of version v3.0.3. In this version, ForeignLinkerBLAS has be removed.
https://github.com/apache/spark/blob/master/pom.xml#L191

Double check (jdk.incubator.foreign cannot be found in the netlib source code):

So we can completely remove options jdk.incubator.foreign and -Dforeign.restricted=warn.

B.For JDK 21
(PS: This is to explain the historical reasons for the differences between the current code logic and the initial ones)
(Just because Spark made changes to support JDK 21)
https://issues.apache.org/jira/browse/SPARK-44088

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Manually test
Pass GA.

Was this patch authored or co-authored using generative AI tooling?

No.

This reverts commit 74fcdb4.

…`JavaModuleOptions`

panbingkun · 2024-04-27T14:16:52Z

cc @luhenry @srowen @zhengruifeng @dongjoon-hyun @LuciferYang

srowen · 2024-04-27T14:34:20Z

Before this flag was gated on Java 21 - it's OK to set this on earlier versions? OK if so

panbingkun · 2024-04-27T14:42:32Z

Before this flag was gated on Java 21 - it's OK to set this on earlier versions? OK if so

Yes, the JDK version of the above manual test environment (local) is 17.

Currently, the version supported by Spark Master is JDK 17 & JDK 21

luhenry · 2024-04-27T14:50:58Z

Thank you for looking into that! Let me know what I should do to update dev.ludovic.netlib further for the needs of Spark

panbingkun · 2024-04-27T15:07:44Z

Thank you for looking into that! Let me know what I should do to update dev.ludovic.netlib further for the needs of Spark

Thank all for providing detailed descriptions during the previous PR submission and review process. That's why I can easily analyze and track the details of history ❤️

zhengruifeng · 2024-04-28T00:23:17Z

also cc @WeichenXu123

dongjoon-hyun

+1, LGTM.

dongjoon-hyun · 2024-04-28T00:39:25Z

Merged to master. Thank you, @panbingkun and all!

LuciferYang · 2024-04-28T02:36:17Z

@panbingkun we should add --add-modules=jdk.incubator.vector to extraJavaTestArgs in pom.xml too

spark/pom.xml

Lines 305 to 323 in 64d3219

    
           <extraJavaTestArgs> 
        
             -XX:+IgnoreUnrecognizedVMOptions 
        
             --add-opens=java.base/java.lang=ALL-UNNAMED 
        
             --add-opens=java.base/java.lang.invoke=ALL-UNNAMED 
        
             --add-opens=java.base/java.lang.reflect=ALL-UNNAMED 
        
             --add-opens=java.base/java.io=ALL-UNNAMED 
        
             --add-opens=java.base/java.net=ALL-UNNAMED 
        
             --add-opens=java.base/java.nio=ALL-UNNAMED 
        
             --add-opens=java.base/java.util=ALL-UNNAMED 
        
             --add-opens=java.base/java.util.concurrent=ALL-UNNAMED 
        
             --add-opens=java.base/java.util.concurrent.atomic=ALL-UNNAMED 
        
             --add-opens=java.base/jdk.internal.ref=ALL-UNNAMED 
        
             --add-opens=java.base/sun.nio.ch=ALL-UNNAMED 
        
             --add-opens=java.base/sun.nio.cs=ALL-UNNAMED 
        
             --add-opens=java.base/sun.security.action=ALL-UNNAMED 
        
             --add-opens=java.base/sun.util.calendar=ALL-UNNAMED 
        
             -Djdk.reflect.useDirectMethodHandle=false 
        
             -Dio.netty.tryReflectionSetAccessible=true 
        
           </extraJavaTestArgs>

panbingkun · 2024-04-28T02:46:19Z

@panbingkun we should add --add-modules=jdk.incubator.vector to extraJavaTestArgs in pom.xml too

spark/pom.xml

Lines 305 to 323 in 64d3219

<extraJavaTestArgs>

-XX:+IgnoreUnrecognizedVMOptions

--add-opens=java.base/java.lang=ALL-UNNAMED

--add-opens=java.base/java.lang.invoke=ALL-UNNAMED

--add-opens=java.base/java.lang.reflect=ALL-UNNAMED

--add-opens=java.base/java.io=ALL-UNNAMED

--add-opens=java.base/java.net=ALL-UNNAMED

--add-opens=java.base/java.nio=ALL-UNNAMED

--add-opens=java.base/java.util=ALL-UNNAMED

--add-opens=java.base/java.util.concurrent=ALL-UNNAMED

--add-opens=java.base/java.util.concurrent.atomic=ALL-UNNAMED

--add-opens=java.base/jdk.internal.ref=ALL-UNNAMED

--add-opens=java.base/sun.nio.ch=ALL-UNNAMED

--add-opens=java.base/sun.nio.cs=ALL-UNNAMED

--add-opens=java.base/sun.security.action=ALL-UNNAMED

--add-opens=java.base/sun.util.calendar=ALL-UNNAMED

-Djdk.reflect.useDirectMethodHandle=false

-Dio.netty.tryReflectionSetAccessible=true

</extraJavaTestArgs>

Okay, let me do it.

…ector` to maven compile args ### What changes were proposed in this pull request? The pr is following up #46246 The pr aims to add `--add-modules=jdk.incubator.vector` to maven `compile args`. ### Why are the changes needed? As commented by LuciferYang , we need to be consistent in `maven` compile. #46246 (comment) <img width="907" alt="image" src="https://github.com/apache/spark/assets/15246973/26163da2-f27d-4ec2-893f-d9282b68aec1"> ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass GA. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #46259 from panbingkun/SPARK-48021. Authored-by: panbingkun <panbingkun@baidu.com> Signed-off-by: yangjie01 <yangjie01@baidu.com>

…`JavaModuleOptions` ### What changes were proposed in this pull request? The pr aims to: - add `--add-modules=jdk.incubator.vector` to `JavaModuleOptions` - remove `jdk.incubator.foreign` and `-Dforeign.restricted=warn` from `SparkBuild.scala` ### Why are the changes needed? 1.`jdk.incubator.vector` First introduction: apache#30810 https://github.com/apache/spark/pull/30810/files#diff-6f545c33f2fcc975200bf208c900a600a593ce6b170180f81e2f93b3efb6cb3e <img width="1045" alt="image" src="https://github.com/apache/spark/assets/15246973/6ac7919a-5d82-475c-b8a2-7d9de71acacc"> Why should we add `--add-modules=jdk.incubator.vector` to `JavaModuleOptions`, Because when we only add `--add-modules=jdk.incubator.vector` to `SparkBuild.scala`, it will only take effect when compiling, as follows: ``` build/sbt "mllib-local/Test/runMain org.apache.spark.ml.linalg.BLASBenchmark" ... ``` <img width="619" alt="image" src="https://github.com/apache/spark/assets/15246973/54d5f55f-cefe-4126-b255-69488f8699a6"> However, when we use `spark-submit`, it is as follows: ``` ./bin/spark-submit --class org.apache.spark.ml.linalg.BLASBenchmark /Users/panbingkun/Developer/spark/spark-community/mllib-local/target/scala-2.13/spark-mllib-local_2.13-4.0.0-SNAPSHOT-tests.jar ``` <img width="1399" alt="image" src="https://github.com/apache/spark/assets/15246973/8e02fa93-fef4-4cdc-96bd-908b3e9baea1"> Obviously, `--add-modules=jdk.incubator.vector` does not take effect in the `Spark runtime`, so I propose adding `--add-modules=jdk.incubator.vector` to the `JavaModuleOptions`(`Spark runtime options`) so that we can improve `performance` by using `hardware-accelerated BLAS operations` by default. After this patch(add `--add-modules=jdk.incubator.vector` to the `JavaModuleOptions`), as follows: <img width="1399" alt="image" src="https://github.com/apache/spark/assets/15246973/da7aa494-0d3c-4c60-9991-e7cd29a1cec5"> 2.`jdk.incubator.foreign` and `-Dforeign.restricted=warn` A.First introduction: apache#32253 https://github.com/apache/spark/pull/32253/files#diff-6f545c33f2fcc975200bf208c900a600a593ce6b170180f81e2f93b3efb6cb3e <img width="1041" alt="image" src="https://github.com/apache/spark/assets/15246973/3f526019-c389-4e60-ab2a-7777f8e99cfb"> Use `dev.ludovic.netlib:blas:1.3.2`, the class `ForeignLinkerBLAS` uses `jdk.incubator.foreign.*` in this version, so we need to add `jdk.incubator.foreign` and `-Dforeign.restricted=warn` to `SparkBuild.scala` https://github.com/apache/spark/pull/32253/files#diff-9c5fb3d1b7e3b0f54bc5c4182965c4fe1f9023d449017cece3005d3f90e8e4d8 <img width="497" alt="image" src="https://github.com/apache/spark/assets/15246973/4fd35e96-0da2-4456-a3f6-6b57ad2e9b64"> https://github.com/luhenry/netlib/blob/v1.3.2/blas/src/main/java/dev/ludovic/netlib/blas/ForeignLinkerBLAS.java#L36 <img width="743" alt="image" src="https://github.com/apache/spark/assets/15246973/4b7e3bd1-4650-4c7d-bdb4-c1761d48d478"> However, with the iterative development of `dev.ludovic.netlib`, `ForeignLinkerBLAS` has experienced one `major` change, as following: luhenry/netlib@48e923c <img width="452" alt="image" src="https://github.com/apache/spark/assets/15246973/7ba30b19-00c7-4cc4-bea7-a6ab4b326ad8"> From now on (V3.0.0), `jdk.incubator.foreign.*` will not be used in `dev.ludovic.netlib` Currently, Spark has used the `dev.ludovic.netlib` of version `v3.0.3`. In this version, `ForeignLinkerBLAS` has be removed. https://github.com/apache/spark/blob/master/pom.xml#L191 Double check (`jdk.incubator.foreign` cannot be found in the `netlib` source code): <img width="674" alt="image" src="https://github.com/apache/spark/assets/15246973/5c6c6d73-6a5d-427a-9fb4-f626f02335ca"> So we can completely remove options `jdk.incubator.foreign` and `-Dforeign.restricted=warn`. B.For JDK 21 (PS: This is to explain the historical reasons for the differences between the current code logic and the initial ones) (Just because `Spark` made changes to support `JDK 21`) https://issues.apache.org/jira/browse/SPARK-44088 <img width="1350" alt="image" src="https://github.com/apache/spark/assets/15246973/34e7e7e8-4e72-470e-abc0-d79406ad25e5"> ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? - Manually test - Pass GA. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#46246 from panbingkun/test_spark_build. Authored-by: panbingkun <panbingkun@baidu.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

…ector` to maven compile args ### What changes were proposed in this pull request? The pr is following up apache#46246 The pr aims to add `--add-modules=jdk.incubator.vector` to maven `compile args`. ### Why are the changes needed? As commented by LuciferYang , we need to be consistent in `maven` compile. apache#46246 (comment) <img width="907" alt="image" src="https://github.com/apache/spark/assets/15246973/26163da2-f27d-4ec2-893f-d9282b68aec1"> ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass GA. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#46259 from panbingkun/SPARK-48021. Authored-by: panbingkun <panbingkun@baidu.com> Signed-off-by: yangjie01 <yangjie01@baidu.com>

test

74fcdb4

github-actions bot added the BUILD label Apr 26, 2024

panbingkun added 2 commits April 27, 2024 17:56

Revert "test"

6f169b6

This reverts commit 74fcdb4.

[SPARK-48021][ML][BUILD] Add --add-modules=jdk.incubator.vector to …

86a88b2

…`JavaModuleOptions`

panbingkun changed the title ~~[Only Test]~~ [SPARK-48021][ML][BUILD] Add --add-modules=jdk.incubator.vector to JavaModuleOptions Apr 27, 2024

panbingkun marked this pull request as ready for review April 27, 2024 14:14

dongjoon-hyun approved these changes Apr 28, 2024

View reviewed changes

dongjoon-hyun closed this in 64d3219 Apr 28, 2024

panbingkun mentioned this pull request Apr 28, 2024

[SPARK-48021][ML][BUILD][FOLLOWUP] add --add-modules=jdk.incubator.vector to maven compile args #46259

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-48021][ML][BUILD] Add `--add-modules=jdk.incubator.vector` to `JavaModuleOptions` #46246

[SPARK-48021][ML][BUILD] Add `--add-modules=jdk.incubator.vector` to `JavaModuleOptions` #46246

panbingkun commented Apr 26, 2024 •

edited

Loading

panbingkun commented Apr 27, 2024

srowen commented Apr 27, 2024

panbingkun commented Apr 27, 2024 •

edited

Loading

luhenry commented Apr 27, 2024

panbingkun commented Apr 27, 2024 •

edited

Loading

zhengruifeng commented Apr 28, 2024

dongjoon-hyun left a comment

dongjoon-hyun commented Apr 28, 2024

LuciferYang commented Apr 28, 2024 •

edited

Loading

panbingkun commented Apr 28, 2024

[SPARK-48021][ML][BUILD] Add --add-modules=jdk.incubator.vector to JavaModuleOptions #46246

[SPARK-48021][ML][BUILD] Add --add-modules=jdk.incubator.vector to JavaModuleOptions #46246

Conversation

panbingkun commented Apr 26, 2024 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

panbingkun commented Apr 27, 2024

srowen commented Apr 27, 2024

panbingkun commented Apr 27, 2024 • edited Loading

luhenry commented Apr 27, 2024

panbingkun commented Apr 27, 2024 • edited Loading

zhengruifeng commented Apr 28, 2024

dongjoon-hyun left a comment

Choose a reason for hiding this comment

dongjoon-hyun commented Apr 28, 2024

LuciferYang commented Apr 28, 2024 • edited Loading

panbingkun commented Apr 28, 2024

[SPARK-48021][ML][BUILD] Add `--add-modules=jdk.incubator.vector` to `JavaModuleOptions` #46246

[SPARK-48021][ML][BUILD] Add `--add-modules=jdk.incubator.vector` to `JavaModuleOptions` #46246

panbingkun commented Apr 26, 2024 •

edited

Loading

panbingkun commented Apr 27, 2024 •

edited

Loading

panbingkun commented Apr 27, 2024 •

edited

Loading

LuciferYang commented Apr 28, 2024 •

edited

Loading