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

fix(udf): fix memory leak in Java UDF #13789

Merged
merged 2 commits into from
Dec 4, 2023
Merged

Conversation

wangrunji0408
Copy link
Contributor

@wangrunji0408 wangrunji0408 commented Dec 4, 2023

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

fix #11508

This PR fixes Arrow memory leak in Java UDF. It creates a child allocator for each RPC and uses try-with-resources syntax to ensure the buffer is recycled timely.

With this patch, running the UDF mentioned in #11508 consumes ~800M memory steadily on my Mac.

Reference: https://arrow.apache.org/docs/java/memory.html#arrowbuf

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • I have added test labels as necessary. See details.
  • I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
  • My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
  • All checks passed in ./risedev check (or alias, ./risedev c)
  • My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)
  • My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

  • My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

@github-actions github-actions bot added the type/fix Bug fix label Dec 4, 2023
Copy link

codecov bot commented Dec 4, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2461651) 68.23% compared to head (c3fce5f) 68.23%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #13789   +/-   ##
=======================================
  Coverage   68.23%   68.23%           
=======================================
  Files        1524     1524           
  Lines      262203   262203           
=======================================
+ Hits       178904   178922   +18     
+ Misses      83299    83281   -18     
Flag Coverage Δ
rust 68.23% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@xxchan xxchan left a comment

Choose a reason for hiding this comment

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

Amazing!

Reference: arrow.apache.org/docs/java/memory.html#arrowbuf

Which part is the most relevant?


Oh, I think it is:

close() allocators after use (whether they are child allocators or the RootAllocator), either manually or preferably via a try-with-resources statement.

Copy link
Member

@xxchan xxchan left a comment

Choose a reason for hiding this comment

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

I'm still not 100% sure that create an allocator per call is the intended usage... (e.g., it also mentions that "Because direct memory is expensive to allocate and deallocate, allocators may share direct buffers", so will this have performance influences?) (Besides, IIUC this is just "not deallocating fast enough" instead of "memory leak"? Will it also lead to memory leak if not that much requests?)

But LGTM since it can fix the issue.

@wangrunji0408
Copy link
Contributor Author

According to the document:

Child allocators are not strictly required, but can help better organize code. For instance, a lower memory limit can be set for a particular section of code. The child allocator can be closed when that section completes, at which point it checks that that section didn’t leak any memory. Child allocators can also be named, which makes it easier to tell where an ArrowBuf came from during debugging.

I feel that it is encouraged to use child allocators. At least it helped me diagnose and eventually fix the memory leak. As for the performance, I'm also not sure if the overhead is significant. If so, we can remove the child allocator.

Besides, IIUC this is just "not deallocating fast enough" instead of "memory leak"? Will it also lead to memory leak if not that much requests?

Maybe. We have not tested whether a manual GC can reduce the memory consumption. But from a developer's view, it is memory leak. 😇

@wangrunji0408 wangrunji0408 added this pull request to the merge queue Dec 4, 2023
Merged via the queue into main with commit 4297914 Dec 4, 2023
26 of 27 checks passed
@wangrunji0408 wangrunji0408 deleted the wrj/fix-java-udf-oom branch December 4, 2023 14:29
wangrunji0408 added a commit that referenced this pull request Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/fix Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Java UDF OOM easily
2 participants