Skip to content

Commit

Permalink
Removing inherited background color optimization from RCTText
Browse files Browse the repository at this point in the history
Summary:
This is a long story. Awhile ago awesome Nick Lockwood (Hey Nick!) introduced a special optimization for ReactNative rendering layer called "inherited background color".
He described this idea in D2811031:
>>>
Blending semitransparent pixels against their background is fairly a fairly expensive operation on mobile GPUs. To reduce blending, React Native has a system called "background color propagation", where the background color of parent views is automatically inherited by child views unless explicitly overridden. This means that translucent pixels can be blended directly against a known background color, avoiding the need to do this dynamically on the GPU.
In practice, this is only useful for views that do their own drawing, which is basically just <Image/> and <Text/> components, and for image components it only really matters when the image has an alpha component.
The automatic background propagation is a bit of a hack, and often does the wrong thing - for example if a view overflows its bounds, or if it overlaps a sibling, the background color will often be incorrect and need to be manually disabled. Because the only place that it provides a significant performance benefit is for text, this diff disables the behavior for everything except <Text/> nodes. It might still be useful for <Image/> nodes too, but looking through the examples in UIExplorer, the number of places where it does the wrong thing for images outnumbers the cases where it provides significant reduction in blending.

However. I think it is time to remove it. Why? There are several reasons:

* It drastically complicates rendering layer. DRASTICALLY. In many many unrelated places (try search for "backgroundColor"!);
* This mechanism is totally non-conceptual to RN and it prevents us to implement some new possible render optimization that we plan to do;
* This adopted only by two components now: Text and ART;
* This is not a significant performance drain anymore; from iOS 6 even UILabel has clear background color by default.
* I doubt that it even works now because `drawRect:` in Text component does not call super method.

So, this diff just turns this feature off for Text. If all performance metrics are neutral, I will delete this mechanism.
Peace.

Reviewed By: sahrens

Differential Revision: D6564199

fbshipit-source-id: 70524fdd955ca32bbf86d2d1ff5e73316b791219
  • Loading branch information
shergin authored and facebook-github-bot committed Dec 15, 2017
1 parent d326c86 commit 8c8944c
Show file tree
Hide file tree
Showing 25 changed files with 0 additions and 5 deletions.
5 changes: 0 additions & 5 deletions Libraries/Text/RCTText.m
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,6 @@ - (void)reactSetFrame:(CGRect)frame
}];
}

- (void)reactSetInheritedBackgroundColor:(UIColor *)inheritedBackgroundColor
{
self.backgroundColor = inheritedBackgroundColor;
}

- (void)didUpdateReactSubviews
{
// Do nothing, as subviews are managed by `setTextStorage:` method
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified ...eferenceImages/RNTester-js-RNTesterApp.ios/[email protected]
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified ...eferenceImages/RNTester-js-RNTesterApp.ios/[email protected]
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.

3 comments on commit 8c8944c

@janicduplessis
Copy link
Contributor

Choose a reason for hiding this comment

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

🙏 As much as I understand the perf optimisation this was definitely annoying especially when developing on Android then coming back on iOS with a bunch of broken text with a random background.

Posted about this a while back #7964

@janicduplessis
Copy link
Contributor

Choose a reason for hiding this comment

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

@shergin Curious about what type of rendering optimisation you are looking at. View flattening / custom drawing à la litho?

@shergin
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@janicduplessis We are thinking about many types of conceptual changes in RN rendering model and about how UIManager communicates with React. But, before we even can think about it concretely, I have to remove a lot of... technical debt... from RN rendering infra. My goal is to have really simple, logically clean rendering layer. Even this work itself should bring significant performance benefits. I hope.

Please sign in to comment.