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.

Sign up for GitHub

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

Jump to bottom

[GLUTEN-6148][CORE] Simplify JniLibLoader loading mechanism for native libraries #6791

Merged
merged 4 commits into from
Aug 14, 2024
Merged

[GLUTEN-6148][CORE] Simplify JniLibLoader loading mechanism for native libraries #6791

merged 4 commits into from
Aug 14, 2024

Conversation

ArnavBalyan
Copy link
Contributor

@ArnavBalyan ArnavBalyan commented Aug 12, 2024 •
edited
Loading

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, needed some feedback on how we can test it better

@github-actions github-actions bot added CORE works for Gluten Core VELOX labels Aug 12, 2024
@github-actions GitHub Actions
Copy link

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:

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@ArnavBalyan ArnavBalyan changed the title [VL] Simplify JniLibLoader Loading Mechanism for Native Libraries [VL-6148] Simplify JniLibLoader loading mechanism for native libraries Aug 12, 2024
@PHILO-HE PHILO-HE changed the title [VL-6148] Simplify JniLibLoader loading mechanism for native libraries [GLUTEN-6148] Simplify JniLibLoader loading mechanism for native libraries Aug 12, 2024
@github-actions GitHub Actions
Copy link

#6148

zhztheplayer
zhztheplayer previously approved these changes Aug 13, 2024
View reviewed changes
Copy link
Member

@zhztheplayer zhztheplayer left a comment

Choose a reason for hiding this comment

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

Just a little minors. Thanks.

gluten-core/src/main/java/org/apache/gluten/vectorized/JniLibLoader.java Outdated
abort();
throw new GlutenException(e);
public void loadAndCreateLink(String libName, String linkName, boolean requireUnload) {
sync.lock();
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good catch, updated thanks

gluten-core/src/main/java/org/apache/gluten/vectorized/JniLibLoader.java Outdated
@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we keep the ctor package-private? If no other places are calling it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done thanks!

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

zhztheplayer
zhztheplayer approved these changes Aug 14, 2024
View reviewed changes
Copy link
Member

@zhztheplayer zhztheplayer left a comment

Choose a reason for hiding this comment

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

I see there are still some unnecessary synchronizations but should be benign overall. So let's merge this first. Thank you for the effort.

@zhztheplayer zhztheplayer merged commit ef5f01c into apache:main Aug 14, 2024
43 checks passed
@zhztheplayer zhztheplayer changed the title [GLUTEN-6148] Simplify JniLibLoader loading mechanism for native libraries [GLUTEN-6148][CORE] Simplify JniLibLoader loading mechanism for native libraries Aug 14, 2024
@ArnavBalyan
Copy link
Contributor Author

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

@wForget wForget mentioned this pull request Sep 14, 2024
Closed
sharkdtu pushed a commit to sharkdtu/gluten that referenced this pull request Nov 11, 2024
@ArnavBalyan
[GLUTEN-6148][CORE] Simplify JniLibLoader loading mechanism for nativ…
fd73954 
…e libraries (apache#6791)

Closes apache#6148
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhztheplayer zhztheplayer zhztheplayer approved these changes

Assignees
No one assigned
Labels
CORE works for Gluten Core VELOX
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[VL] Simplify JniLibLoader
2 participants
@ArnavBalyan @zhztheplayer