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

Clean up Rust/Go interop #10915

Merged
merged 2 commits into from
Mar 7, 2022
Merged

Clean up Rust/Go interop #10915

merged 2 commits into from
Mar 7, 2022

Conversation

zmb3
Copy link
Collaborator

@zmb3 zmb3 commented Mar 7, 2022

This PR introduces 2 changes:

  • fixes a memory leak for desktop bitmaps sent from Rust to Go
  • optimization: reduce the number of times bitmap data is copied

Copy link
Contributor

@xacrimon xacrimon left a comment

Choose a reason for hiding this comment

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

lgtm

@zmb3 zmb3 force-pushed the zmb3/rust-memory-leak branch from 5e86ce0 to c010eaf Compare March 7, 2022 20:12
@zmb3 zmb3 enabled auto-merge (rebase) March 7, 2022 20:13
@zmb3 zmb3 requested a review from ibeckermayer March 7, 2022 21:25
zmb3 added 2 commits March 7, 2022 14:25
Prior to this change, the bitmap data was copied into Go first
(via C.GoBytes), converted to RGBA, and then copied a second time
into the RBGA image that we pass to the PNG encoder.

Instead, use unsafe.Slice to get a slice backed by the Rust-managed
memory. Perform the RGBA conversion in-place here, and then perform
one copy from the Rust-managed memory into the Go RBGA image.
Pass Go a pointer to the CGOBitmap, otherwise Rust assumes ownership
is passing through the FFI and will not free the memory when we're
finished with it.
@zmb3 zmb3 force-pushed the zmb3/rust-memory-leak branch from c010eaf to 0b37060 Compare March 7, 2022 21:25
@zmb3 zmb3 merged commit 276091c into master Mar 7, 2022
@zmb3 zmb3 deleted the zmb3/rust-memory-leak branch March 7, 2022 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants