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

Jump to bottom

Memory leak: org.sqlite.JDBC holds classloader of KSP #1063

Closed
ting-yuan opened this issue Aug 13, 2022 · 5 comments
Closed

Memory leak: org.sqlite.JDBC holds classloader of KSP #1063

ting-yuan opened this issue Aug 13, 2022 · 5 comments

Comments

@ting-yuan
Copy link
Collaborator

ting-yuan commented Aug 13, 2022 •
edited

This happens when running Room:

  1. Kotlin compiler daemon loads compiler plugins (KSP) with a new class loader every time it gets a request from Gradle.
  2. KSP loads processors(Room) with a child classloader.
  3. Room uses org.sqlite.JDBC, which is also loaded with Room's classloader.
  4. org.sqlite.JDBC's static initializer calls java.sql.DriverManager.registerDriver, which stores the class org.sqlite.JDBC in a static member of java.sql.DriverManager.
  5. java.sql.DriverManager is loaded by the root classloader.
  6. Therefore all everything loaded with KSP's classloader can also be reached by the root classloader and never be GC'ed.

Here is a sample project thanks to @BugsBunnyBR.

Screenshot 2022-08-13 01 37 09
@ting-yuan ting-yuan mentioned this issue Aug 15, 2022
Merged
@juliocbcotta
Copy link

@ting-yuan @neetopia
Thank you for your analysis and mitigation efforts!
It seems that more could be done on Room side of things, so I opened this issue on Room issue tracker. Maybe you folks could contribute with your thoughts there ?

@juliocbcotta
Copy link

Should we try to open an issue on Sqlite too?

copybara-service bot pushed a commit to androidx/androidx that referenced this issue Aug 16, 2022
@danysantiago
Unregister SQLite JDBC driver to avoid memory leaks.
0bd479c 
The SQLite library used by Room registers the JDBC driver in a static block and thus conveniently has the driver available but never unregister it. In a normal application, this would be fine since they tend to have a single class loader, but in Gradle builds, multiple class loaders are used and the driver is registered multiple times without ever being unregistered, this can lead to memory leaks.

To avoid leaks, Room tries to manage the life of the driver by unregistering it once processing is done. In the case that the same instance of Room is used to do new processing, Room re-registers the driver since using the same instance of Room would mean the same class loader and JDBC's static block and driver registering logic would not execute. Re-register a driver that is already present is a no-op.

See: google/ksp#1063

Test: Verified manually with sample project in linked bug.
Change-Id: I8916fd0bcb42337314feebef9afa0b54d4f479bc
@ting-yuan
Copy link
Collaborator Author

This is fixed in Room already. It is also mitigated in #1067 in case there are other processors with similar issue.

@juliocbcotta
Copy link

To the people watching this issue...

I ran the benchmark in a branch with the latest version of room and these are the results.
Screenshot 2022-08-25 at 15 49 40

This is way better than before, even though it seems to use lots of memory I was able to run 50 iterations and didn't get an out of memory. 👏 👏
I will be waiting a version of ksp to verify the final results.

@juliocbcotta
Copy link

juliocbcotta commented Oct 16, 2022 •
edited

So, I updated ksp/kotlin/room to the latest version and ran the benchmark again. The behaviour of the memory consumption improved by a lot! Not only that, but the number of live threads stabilised too. Great work folks! 👏 👏 🍰

Screenshot 2022-10-16 at 12 40 38

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@juliocbcotta @ting-yuan