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(iOS): Replace uses of CGColorRef with UIColor to avoid manual memory management #46847

Closed
wants to merge 4 commits into from

Conversation

Saadnajmi
Copy link
Contributor

@Saadnajmi Saadnajmi commented Oct 4, 2024

Summary:

Update React Native on iOS to do less manual memory management, by replacing uses of CGColorRef with UIColor, and only calling UIColor.CGColor when needed. This results in much less manual memory management, which is probably a good thing in 2024 :D. The downside is there is a breaking change: the signature of a method in RCTBorderDrawing changes.

This is a followup to #46797 . After that PR merged and I tested merging React Native macOS to the 0.76-stable branch cut commit again, I saw even more places I needed to manually call CGColorRetain / CGColorRelease. The reason is due to React Native macOS specifics (explained below) and I could update React Native macOS to not need these changes, but I thought I would at least throw up a PR to propose the changes, as it may be good to move away from using Core Graphics' C base API as much as possible.

Longer Explanation

With microsoft#2209 , I wrote a shim of UIGraphicsImageRenderer for macOS. The main difference is my shim calls the NSImage API imageWithSize:flipped:drawingHandler: to shim the UIGraphicsImageRenderer API imageWithData:. The difference between the two is that the macOS API copies the block, and executes it when Appkit is about to draw the image, while the iOS API executes the block right away. Because of this, I think I am hitting way more places where CGColorRef variables needed to be retained / released. Additionally, I hit more of them when I merge to the 0.76 branch cut commit (this is why I didn't catch it with #46797).

Given this constraint, I have a couple of options:

  1. Refactor my macOS shim to use the deprecated API [NSImage lockFocus]
    • I am not a fan of this because lockFocus was deprecated for imageWithSize:flipped:drawingHandler:
  2. Refactor my macOS shim to do what we used to do: Create a CGContext manually and write it to an image
    • This is probably OK. Relies on less Appkit specifics, and potentially gives more control to RN on the rendering layer, which I recall @lenaic told me is better for Fabric)
  3. Refactor React Native to avoid CGColorRef altogether, and use UIColor (which ARC will memory manage for us) as much as possible.
    • This is the approach of this PR and my preferred approach. The downside is this changes the signature of some of the methods in RCTBorderDrawing which is a breaking change. I've seen other PRs do this, so maybe its OK?

Changelog:

[IOS] [BREAKING] - Replace uses of CGColorRef with UIColor to avoid manual memory management

Test Plan:

Launching RNTester's View example (which tests a lot of the border / outline / shadow rendering) does not crash for me on both iOS and macOS.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Microsoft Partner: Microsoft Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Oct 4, 2024
@facebook-github-bot
Copy link
Contributor

@joevilches has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@joevilches
Copy link
Contributor

Hey @Saadnajmi, this looks good to me, thanks for taking this on!

@joevilches
Copy link
Contributor

@Saadnajmi there are some build errors internally that coincide with the warnings about incompatible pointer on RCTView.m, mind addressing that? (I would comment on the lines but github is not letting me for some reason -_-)

@Saadnajmi
Copy link
Contributor Author

@Saadnajmi there are some build errors internally that coincide with the warnings about incompatible pointer on RCTView.m, mind addressing that? (I would comment on the lines but github is not letting me for some reason -_-)

Eh, I'm not sure I can see those errors, but I'll take a second look. I may have just been surprising them.

@joevilches
Copy link
Contributor

@Saadnajmi GitHub added warnings here that coincide with the error. Screenshotting since I still cannot add inline comment
Screenshot 2024-10-07 at 15 01 31

@Saadnajmi
Copy link
Contributor Author

@joevilches OK I see them now, and they are fixed. Totally just missed / ignored those, thanks!

@facebook-github-bot
Copy link
Contributor

@joevilches has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Oct 9, 2024
@facebook-github-bot
Copy link
Contributor

@joevilches merged this pull request in b70709d.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @Saadnajmi in b70709d

When will my fix make it into a release? | How to file a pick request?

@Saadnajmi Saadnajmi deleted the avoid-core-graphics branch October 10, 2024 00:30
Saadnajmi added a commit to Saadnajmi/react-native-macos that referenced this pull request Oct 10, 2024
…emory management (facebook#46847)

Summary:
Update React Native on iOS to do less manual memory management, by replacing uses of `CGColorRef` with `UIColor`, and only calling `UIColor.CGColor` when needed. This results in much less manual memory management, which is probably a good thing in 2024 :D. The downside is there is a breaking change: the signature of a method in `RCTBorderDrawing` changes.

This is a followup to facebook#46797 . After that PR merged and I tested merging React Native macOS to the `0.76-stable` branch cut commit again, I saw even more places I needed to manually call `CGColorRetain` / `CGColorRelease`. The reason is due to React Native macOS specifics (explained below) and I could update React Native macOS to not need these changes, but I thought I would at least throw up a PR to propose the changes, as it may be good to move away from using Core Graphics' C base API as much as possible.

With microsoft#2209 , I wrote a shim of [UIGraphicsImageRenderer](https://developer.apple.com/documentation/uikit/uigraphicsimagerenderer) for macOS. The main difference is my shim calls the NSImage API [imageWithSize:flipped:drawingHandler:](https://developer.apple.com/documentation/appkit/nsimage/1519860-imagewithsize?language=objc) to shim the UIGraphicsImageRenderer API [imageWithData:](https://developer.apple.com/documentation/uikit/uiimage/1624137-imagewithdata). The difference between the two is that the macOS API copies the block, and executes it when Appkit is about to draw the image, while the iOS API executes the block right away. Because of this, I think I am hitting way more places where `CGColorRef` variables needed to be retained / released. Additionally, I hit more of them when I merge to the 0.76 branch cut commit (this is why I didn't catch it with facebook#46797).

Given this constraint, I have a couple of options:
1. Refactor my macOS shim to use the deprecated API [[NSImage lockFocus]](https://developer.apple.com/documentation/appkit/nsimage/1519891-lockfocus)
    - I am not a fan of this because `lockFocus` was deprecated for `imageWithSize:flipped:drawingHandler:`
2. Refactor my macOS shim to do what we used to do: Create a CGContext manually and write it to an image
    - This is probably OK. Relies on less Appkit specifics, and potentially gives more control to RN on the rendering layer, which I recall lenaic told me is better for Fabric)
3. Refactor React Native to avoid CGColorRef altogether, and use `UIColor` (which ARC will memory manage for us) as much as possible.
    - This is the approach of this PR and my preferred approach. The downside is this changes the signature of some of the methods in `RCTBorderDrawing` which is a breaking change. I've seen other PRs do this, so maybe its OK?

[IOS] [BREAKING] -  Replace uses of `CGColorRef` with UIColor to avoid manual memory management

Pull Request resolved: facebook#46847

Test Plan: Launching RNTester's View example (which tests a lot of the border / outline / shadow rendering) does not crash for me on both iOS and macOS.

Reviewed By: NickGerleman

Differential Revision: D63989547

Pulled By: joevilches

fbshipit-source-id: 5e85e17567e3dd8b4b0388452398954ad2e02464
@facebook-github-bot
Copy link
Contributor

This pull request has been reverted by b0e2cb3.

Saadnajmi added a commit to microsoft/react-native-macos that referenced this pull request Oct 13, 2024
…him (#2219)

* Rename `RCTUIGraphicsImageRenderer` to `RCTMakeUIGraphicsImageRenderer` (facebook#46772)

Summary:
Because `UIGraphicsImageRenderer` doesn't exist on macOS, I need to shim it for React Native macOS (See #2209). I planned to use the name `RCTUIGraphicsImageRenderer`. However.. it seems that is used by a static helper function in `RCTBorderDrawing.m`. So.. let's rename it? The function is just a helper method to make an instance of the class, so I think the name `RCTMakeUIGraphicsImageRenderer` is slightly more idiomatic anyway.

This method is not public, so it should not break the public API of React Native.

[IOS] [CHANGED] - Rename `RCTUIGraphicsImageRenderer` to `RCTMakeUIGraphicsImageRenderer`

Pull Request resolved: facebook#46772

Test Plan: CI should pass

Reviewed By: joevilches

Differential Revision: D63765490

Pulled By: cipolleschi

fbshipit-source-id: de68dce0f92ec249ea8586dbf7b9ba34a8476074

* fix(iOS): Replace uses of `CGColorRef` with UIColor to avoid manual memory management (facebook#46847)

Summary:
Update React Native on iOS to do less manual memory management, by replacing uses of `CGColorRef` with `UIColor`, and only calling `UIColor.CGColor` when needed. This results in much less manual memory management, which is probably a good thing in 2024 :D. The downside is there is a breaking change: the signature of a method in `RCTBorderDrawing` changes.

This is a followup to facebook#46797 . After that PR merged and I tested merging React Native macOS to the `0.76-stable` branch cut commit again, I saw even more places I needed to manually call `CGColorRetain` / `CGColorRelease`. The reason is due to React Native macOS specifics (explained below) and I could update React Native macOS to not need these changes, but I thought I would at least throw up a PR to propose the changes, as it may be good to move away from using Core Graphics' C base API as much as possible.

With #2209 , I wrote a shim of [UIGraphicsImageRenderer](https://developer.apple.com/documentation/uikit/uigraphicsimagerenderer) for macOS. The main difference is my shim calls the NSImage API [imageWithSize:flipped:drawingHandler:](https://developer.apple.com/documentation/appkit/nsimage/1519860-imagewithsize?language=objc) to shim the UIGraphicsImageRenderer API [imageWithData:](https://developer.apple.com/documentation/uikit/uiimage/1624137-imagewithdata). The difference between the two is that the macOS API copies the block, and executes it when Appkit is about to draw the image, while the iOS API executes the block right away. Because of this, I think I am hitting way more places where `CGColorRef` variables needed to be retained / released. Additionally, I hit more of them when I merge to the 0.76 branch cut commit (this is why I didn't catch it with facebook#46797).

Given this constraint, I have a couple of options:
1. Refactor my macOS shim to use the deprecated API [[NSImage lockFocus]](https://developer.apple.com/documentation/appkit/nsimage/1519891-lockfocus)
    - I am not a fan of this because `lockFocus` was deprecated for `imageWithSize:flipped:drawingHandler:`
2. Refactor my macOS shim to do what we used to do: Create a CGContext manually and write it to an image
    - This is probably OK. Relies on less Appkit specifics, and potentially gives more control to RN on the rendering layer, which I recall lenaic told me is better for Fabric)
3. Refactor React Native to avoid CGColorRef altogether, and use `UIColor` (which ARC will memory manage for us) as much as possible.
    - This is the approach of this PR and my preferred approach. The downside is this changes the signature of some of the methods in `RCTBorderDrawing` which is a breaking change. I've seen other PRs do this, so maybe its OK?

[IOS] [BREAKING] -  Replace uses of `CGColorRef` with UIColor to avoid manual memory management

Pull Request resolved: facebook#46847

Test Plan: Launching RNTester's View example (which tests a lot of the border / outline / shadow rendering) does not crash for me on both iOS and macOS.

Reviewed By: NickGerleman

Differential Revision: D63989547

Pulled By: joevilches

fbshipit-source-id: 5e85e17567e3dd8b4b0388452398954ad2e02464

* Followup fixes

* Remove extra scaleFactor argument that is now unused

* Further reduce diffs related to scaleFactor

* fix typos and dead code
joevilches added a commit to joevilches/react-native that referenced this pull request Oct 14, 2024
…emory management v2 (facebook#47019)

Summary:

NOTE: This code is largely the same as facebook#46847, I am merely fixing some internal issues

The original diff (D63989547) was backed out because it was causing some crashes related to some internal code that had to be changed. That code did not necessarily need to be changed - we could have just left the `RCTCreateCGColorRefFromSharedColor` function and not touched the internal code - which is what I am going to do here

Changelog: [Internal]

Differential Revision: D64201626
facebook-github-bot pushed a commit that referenced this pull request Oct 15, 2024
…emory management v2 (#47019)

Summary:
Pull Request resolved: #47019

NOTE: This code is largely the same as #46847, I am merely fixing some internal issues

The original diff (D63989547) was backed out because it was causing some crashes related to some internal code that had to be changed. That code did not necessarily need to be changed - we could have just left the `RCTCreateCGColorRefFromSharedColor` function and not touched the internal code - which is what I am going to do here

Changelog: [Internal]

Reviewed By: NickGerleman

Differential Revision: D64201626

fbshipit-source-id: a8ead7a542514aee0749973e284cb7727d393c03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. p: Microsoft Partner: Microsoft Partner Reverted Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants