You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by "ArnavBalyan (via GitHub)" <gi...@apache.org> on 2024/08/12 13:13:36 UTC
[PR] [VL] Simplify JniLibLoader Loading Mechanism for Native Libraries [incubator-gluten]
ArnavBalyan opened a new pull request, #6791:
URL: https://github.com/apache/incubator-gluten/pull/6791
## What changes were proposed in this pull request?
- Currently Gluten uses an overdesigned library loading mechanism consisting of rollbacks and transactional loading of native libraries.
- However during failures, an exception is thrown and app simply crashes without need for "rollbacks".
- Simplified this design, now we simply load libraries sequentially and maintain the existing behaviour of throwing error when there is an issue with loading the libraries. (This intentionally does not change signature of existing used methods of JniLibLoader to maintain simplicity in the change)
Fixes: #6148
## How was this patch tested?
- Local Spark shell and ran some SparkSQL queries with gluten.
- Unit tests should cover the rest
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-6148] Simplify JniLibLoader loading mechanism for native libraries [incubator-gluten]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #6791:
URL: https://github.com/apache/incubator-gluten/pull/6791#discussion_r1714584283
##########
gluten-core/src/main/java/org/apache/gluten/vectorized/JniLibLoader.java:
##########
@@ -175,160 +150,80 @@ public static synchronized void unloadFromPath(String libPath) {
}
}
- private static final class LoadRequest {
- final String libName;
- final String linkName;
- final boolean requireUnload;
-
- private LoadRequest(String libName, String linkName, boolean requireUnload) {
- this.libName = libName;
- this.linkName = linkName;
- this.requireUnload = requireUnload;
+ public void mapAndLoad(String unmappedLibName, boolean requireUnload) {
+ sync.lock();
+ try {
+ final String mappedLibName = System.mapLibraryName(unmappedLibName);
+ load(mappedLibName, requireUnload);
+ } catch (Exception e) {
+ throw new GlutenException(e);
+ } finally {
+ sync.unlock();
}
}
- private static final class LoadAction {
- final String libName;
- final String linkName;
- final boolean requireUnload;
- final File file;
-
- private LoadAction(String libName, String linkName, boolean requireUnload, File file) {
- this.libName = libName;
- this.linkName = linkName;
- this.requireUnload = requireUnload;
- this.file = file;
- }
-
- public boolean requireLinking() {
- return !Objects.isNull(linkName);
+ public void load(String libName, boolean requireUnload) {
+ sync.lock();
+ try {
+ if (loadedLibraries.contains(libName)) {
+ LOG.debug("Library {} has already been loaded, skipping", libName);
+ return;
+ }
+ File file = moveToWorkDir(workDir, libName);
+ loadWithLink(file.getAbsolutePath(), null, requireUnload);
+ loadedLibraries.add(libName);
+ LOG.info("Successfully loaded library {}", libName);
+ } catch (IOException e) {
+ throw new GlutenException(e);
+ } finally {
+ sync.unlock();
}
}
- public class JniLoadTransaction {
- private final AtomicBoolean finished = new AtomicBoolean(false);
- private final Map<String, LoadRequest> toLoad = new LinkedHashMap<>(); // ordered
-
- private JniLoadTransaction() {
- JniLibLoader.this.sync.lock();
- }
-
- public JniLoadTransaction mapAndLoad(String unmappedLibName, boolean requireUnload) {
- try {
- final String mappedLibName = System.mapLibraryName(unmappedLibName);
- load(mappedLibName, requireUnload);
- return this;
- } catch (Exception e) {
- abort();
- throw new GlutenException(e);
+ public void loadAndCreateLink(String libName, String linkName, boolean requireUnload) {
+ sync.lock();
Review Comment:
Do we still need this lock as class member? Perhaps we can minimize the lock scope by only making operations on `loadedLibraries` atomic. Say `synchronized (loadedLibraries) {...}` may be enough.
##########
gluten-core/src/main/java/org/apache/gluten/vectorized/JniLibLoader.java:
##########
@@ -72,7 +62,7 @@ public class JniLibLoader {
private final Set<String> loadedLibraries = new HashSet<>();
private final Lock sync = new ReentrantLock();
- JniLibLoader(String workDir) {
+ public JniLibLoader(String workDir) {
Review Comment:
Can we keep the ctor package-private? If no other places are calling it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-6148] Simplify JniLibLoader loading mechanism for native libraries [incubator-gluten]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer merged PR #6791:
URL: https://github.com/apache/incubator-gluten/pull/6791
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-6148] Simplify JniLibLoader loading mechanism for native libraries [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #6791:
URL: https://github.com/apache/incubator-gluten/pull/6791#issuecomment-2286507059
Run Gluten Clickhouse CI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Simplify JniLibLoader Loading Mechanism for Native Libraries [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #6791:
URL: https://github.com/apache/incubator-gluten/pull/6791#issuecomment-2283954039
Run Gluten Clickhouse CI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-6148] Simplify JniLibLoader loading mechanism for native libraries [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #6791:
URL: https://github.com/apache/incubator-gluten/pull/6791#issuecomment-2285939383
Run Gluten Clickhouse CI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-6148][CORE] Simplify JniLibLoader loading mechanism for native libraries [incubator-gluten]
Posted by "ArnavBalyan (via GitHub)" <gi...@apache.org>.
ArnavBalyan commented on PR #6791:
URL: https://github.com/apache/incubator-gluten/pull/6791#issuecomment-2287580937
> I see there are still some unnecessary synchronizations but should be benign overall. So let's merge this first. Thank you for the effort.
Ah missed the inner ones, I'll remove in a follow up change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-6148] Simplify JniLibLoader loading mechanism for native libraries [incubator-gluten]
Posted by "ArnavBalyan (via GitHub)" <gi...@apache.org>.
ArnavBalyan commented on code in PR #6791:
URL: https://github.com/apache/incubator-gluten/pull/6791#discussion_r1715078692
##########
gluten-core/src/main/java/org/apache/gluten/vectorized/JniLibLoader.java:
##########
@@ -175,160 +150,80 @@ public static synchronized void unloadFromPath(String libPath) {
}
}
- private static final class LoadRequest {
- final String libName;
- final String linkName;
- final boolean requireUnload;
-
- private LoadRequest(String libName, String linkName, boolean requireUnload) {
- this.libName = libName;
- this.linkName = linkName;
- this.requireUnload = requireUnload;
+ public void mapAndLoad(String unmappedLibName, boolean requireUnload) {
+ sync.lock();
+ try {
+ final String mappedLibName = System.mapLibraryName(unmappedLibName);
+ load(mappedLibName, requireUnload);
+ } catch (Exception e) {
+ throw new GlutenException(e);
+ } finally {
+ sync.unlock();
}
}
- private static final class LoadAction {
- final String libName;
- final String linkName;
- final boolean requireUnload;
- final File file;
-
- private LoadAction(String libName, String linkName, boolean requireUnload, File file) {
- this.libName = libName;
- this.linkName = linkName;
- this.requireUnload = requireUnload;
- this.file = file;
- }
-
- public boolean requireLinking() {
- return !Objects.isNull(linkName);
+ public void load(String libName, boolean requireUnload) {
+ sync.lock();
+ try {
+ if (loadedLibraries.contains(libName)) {
+ LOG.debug("Library {} has already been loaded, skipping", libName);
+ return;
+ }
+ File file = moveToWorkDir(workDir, libName);
+ loadWithLink(file.getAbsolutePath(), null, requireUnload);
+ loadedLibraries.add(libName);
+ LOG.info("Successfully loaded library {}", libName);
+ } catch (IOException e) {
+ throw new GlutenException(e);
+ } finally {
+ sync.unlock();
}
}
- public class JniLoadTransaction {
- private final AtomicBoolean finished = new AtomicBoolean(false);
- private final Map<String, LoadRequest> toLoad = new LinkedHashMap<>(); // ordered
-
- private JniLoadTransaction() {
- JniLibLoader.this.sync.lock();
- }
-
- public JniLoadTransaction mapAndLoad(String unmappedLibName, boolean requireUnload) {
- try {
- final String mappedLibName = System.mapLibraryName(unmappedLibName);
- load(mappedLibName, requireUnload);
- return this;
- } catch (Exception e) {
- abort();
- throw new GlutenException(e);
+ public void loadAndCreateLink(String libName, String linkName, boolean requireUnload) {
+ sync.lock();
Review Comment:
That's a good catch, updated thanks
##########
gluten-core/src/main/java/org/apache/gluten/vectorized/JniLibLoader.java:
##########
@@ -72,7 +62,7 @@ public class JniLibLoader {
private final Set<String> loadedLibraries = new HashSet<>();
private final Lock sync = new ReentrantLock();
- JniLibLoader(String workDir) {
+ public JniLibLoader(String workDir) {
Review Comment:
Done thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Simplify JniLibLoader Loading Mechanism for Native Libraries [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #6791:
URL: https://github.com/apache/incubator-gluten/pull/6791#issuecomment-2283953186
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICEGJ-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
Could you open an issue for this pull request on Github Issues?
https://github.com/apache/incubator-gluten/issues
Then could you also rename ***commit message*** and ***pull request title*** in the following format?
[GLUTEN-${ISSUES_ID}][COMPONENT]feat/fix: ${detailed message}
See also:
* [Other pull requests](https://github.com/apache/incubator-gluten/pulls/)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-6148] Simplify JniLibLoader loading mechanism for native libraries [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #6791:
URL: https://github.com/apache/incubator-gluten/pull/6791#issuecomment-2284008683
https://github.com/apache/incubator-gluten/issues/6148
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org