Skip to content

[VL] Delete global reference to a class object in JNI unload#9268

Merged
philo-he merged 1 commit intoapache:mainfrom
philo-he:delete-global-reference
Apr 9, 2025
Merged

[VL] Delete global reference to a class object in JNI unload#9268
philo-he merged 1 commit intoapache:mainfrom
philo-he:delete-global-reference

Conversation

@philo-he
Copy link
Copy Markdown
Member

@philo-he philo-he commented Apr 9, 2025

What changes were proposed in this pull request?

The global reference to class object of NativePlanValidationInfo was introduced in #9092. It should be explicitly deleted in JNI unload.

@github-actions github-actions bot added the VELOX label Apr 9, 2025
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 9, 2025

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:

@philo-he
Copy link
Copy Markdown
Member Author

philo-he commented Apr 9, 2025

@jackylee-ch, could you please take a look?

@philo-he philo-he merged commit 690b689 into apache:main Apr 9, 2025
47 checks passed
@zhztheplayer
Copy link
Copy Markdown
Member

@philo-he Have you met any issue without the patch?

@philo-he
Copy link
Copy Markdown
Member Author

philo-he commented Apr 9, 2025

@zhztheplayer, no, seems this reference cannot be auto deleted, right? And I note all created global reference in our code are explicitly deleted when JNI is unloading.

@zhztheplayer
Copy link
Copy Markdown
Member

@zhztheplayer, no, seems this reference cannot be auto deleted, right?

For now, actually the unloading is invoked only when JVM is shutting down, so it's not a big deal whether we explicitly delete them at that time. But it's good to keep a consistent form of code. I was just curious whether there was an error or so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants