Remove usages of RTTI in places used by react-native-windows by acoates-ms · Pull Request #31694 · facebook/react-native

acoates-ms · 2021-06-09T22:54:34Z

Summary

Adding runtime type information adds greatly to the binary size, so react-native-windows builds without it. But some parts of the fabric code currently uses dynamic_cast, which means to use fabric we have to build with RTTI turned on. This PR removes the usages of dynamic_cast that are hit in release builds, which should allow react-native-windows to turn off RTTI in release builds.

Required for: microsoft/react-native-windows#7981

One thing to note, the comment in ShadowNodeTraits indicates that core was reserving the first 16 bits. I'm adding two more. Is that ok? Should core be reserving more for future use?

Changelog

[Internal] [Fixed] - Remove uses of dynamic_cast in release builds

Test Plan

Verified that I can build react-native-windows with Fabric in release, without RTTI.
Boot / clicked around in RNW RNTester

analysis-bot · 2021-06-09T23:31:29Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	9,219,560	-915
android	hermes	armeabi-v7a	8,741,757	-733
android	hermes	x86	9,683,008	-660
android	hermes	x86_64	9,644,855	-723
android	jsc	arm64-v8a	10,866,186	-894
android	jsc	armeabi-v7a	9,779,665	-707
android	jsc	x86	10,925,045	-636
android	jsc	x86_64	11,528,422	-689

Base commit: 36c0a7d

analysis-bot · 2021-06-10T02:53:06Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 6e2989a

JoshuaGross · 2021-06-10T05:40:31Z

No concerns from me, this looks good!

sammy-SC · 2021-06-10T11:45:00Z

Hey @acoates-ms

Thank you for the PR! The change looks great, could you add unit tests to https://github.com/facebook/react-native/blob/master/ReactCommon/react/renderer/core/tests/traitCastTest.cpp ?

facebook-github-bot · 2021-06-10T19:51:15Z

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

sammy-SC · 2021-06-10T20:06:47Z

Thank you @acoates-ms.

JoshuaGross · 2021-06-10T21:07:57Z

@acoates-ms When these tests run on iOS I get the error "Death tests are not supported on this platform." - not sure if this is iOS in general or iOS test infra at FB. Can we remove the death tests or gate them to not-iOS?

acoates-ms · 2021-06-10T21:10:06Z

There are already ungated death tests in that file. Is it maybe indicating that the test failed? Or is it really a failure to run the test?

JoshuaGross · 2021-06-10T21:11:34Z

There are already ungated death tests in that file. Is it maybe indicating that the test failed? Or is it really a failure to run the test?

I'm only getting those messages for traitCastTest.cpp for whatever reason...

EDIT: I didn't answer your question, apologies. Here's the error messages:

[WARNING]
xplat/js/react-native-github/ReactCommon/react/renderer/core/tests/traitCastTest.cpp:105:
:
Death tests are not supported on this platform.
Statement 'traitCast<LayoutableShadowNode const &>(*rawTextShadowNode)' cannot be verified.

[WARNING]
[WARNING] xplat/js/react-native-github/ReactCommon/react/renderer/core/tests/traitCastTest.cpp:105:: Death tests are not supported on this platform.
Statement 'traitCast<LayoutableShadowNode const &>(*rawTextShadowNode)' cannot be verified.

xplat/js/react-native-github/ReactCommon/react/renderer/core/tests/traitCastTest.cpp:107:: Death tests are not supported on this platform.
Statement 'traitCast<YogaLayoutableShadowNode const &>(*rawTextShadowNode)' cannot be verified.
[WARNING]
[WARNING] xplat/js/react-native-github/ReactCommon/react/renderer/core/tests/traitCastTest.cpp:107:: Death tests are not supported on this platform.
Statement 'traitCast<YogaLayoutableShadowNode const &>(*rawTextShadowNode)' cannot be verified.

Edit 2: those are truncated, there are more but they all look similar to that ^

JoshuaGross · 2021-06-10T21:13:55Z

@acoates-ms Okay, let's try this again :) those are just the warning messages so probably not relevant. Here's the crash.

WARNING: Logging before InitGoogleLogging() is written to STDERR
E0610 13:52:53.158354 326520256 RawTextShadowNode.h:45] react_native_assert failure: castable
WARNING: Logging before InitGoogleLogging() is written to STDERR
E0610 13:52:53.158354 326520256 RawTextShadowNode.h:45] react_native_assert failure: castable
Assertion failed: (castable), fu
nction traitCast, file buck-out/
gen/33fbdb84/xplat/js/react-nati
ve-github/ReactCommon/react/rend
erer/components/text/textApple#header-mode-symlink-tree-with-header-map,headers/react/renderer/components/text/RawTextShadowNode.h, line 45.
Assertion failed: (castable), function traitCast, file buck-out/gen/33fbdb84/xplat/js/react-native-github/ReactCommon/react/renderer/components/text/textApple#header-mode-symlink-tree-with-header-map,headers/react/renderer/components/text/RawTextShadowNode.h, line 45.

acoates-ms · 2021-06-10T21:21:31Z

Ah I think I see it. Sorry I haven't actually run these UTs -- Not sure if they are runnable in OSS. -- Fix incoming. I have some expectations inverted in the tests.

facebook-github-bot · 2021-06-10T21:39:17Z

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

JoshuaGross · 2021-06-10T22:40:12Z

Ah I think I see it. Sorry I haven't actually run these UTs -- Not sure if they are runnable in OSS. -- Fix incoming. I have some expectations inverted in the tests.

Oops, looks like the test doesn't compile now. Here's a truncated error, hopefully it's enough info:

Summary: 
xplat/js/react-native-github/ReactCommon/react/renderer/core/tests/traitCastTest.cpp:114:78: error: too many arguments provided to function-like macro invocation
      traitCast<RawTextShadowNode const &>(*shadowNodeForRawTextShadowNode), "");
                                                                             ^
xplat/js/react-native-github/ReactCommon/react/renderer/core/tests/traitCastTest.cpp:113:3: error: use of undeclared identifier 'EXPECT_NO_FATAL_FAILURE'
  EXPECT_NO_FATAL_FAILURE(
  ^
xplat/js/react-native-github/ReactCommon/react/renderer/core/tests/traitCastTest.cpp:118:73: error: too few arguments provided to function-like macro invocation
      traitCast<RawTextShadowNode const &>(*shadowNodeForTextShadowNode));
                                                                        ^
xplat/js/react-native-github/ReactCommon/react/renderer/core/tests/traitCastTest.cpp:117:3: error: use of undeclared identifier 'EXPECT_DEATH_IF_SUPPORTED'
  EXPECT_DEATH_IF_SUPPORTED(
  ^
xplat/js/react-native-github/ReactCommon/react/renderer/core/tests/traitCastTest.cpp:122:60: error: too few arguments provided to function-like macro invocation
      traitCast<RawTextShadowNode const &>(*viewShadowNode));
                                                           ^
xplat/js/react-native-github/ReactCommon/react/renderer/core/tests/traitCastTest.cpp:121:3: error: use of undeclared identifier 'EXPECT_DEATH_IF_SUPPORTED'
  EXPECT_DEATH_IF_SUPPORTED(
  ^
xplat/js/react-native-github/ReactCommon/react/renderer/core/tests/traitCastTest.cpp:129:72: error: too many arguments provided to function-like macro invocation
      traitCast<TextShadowNode const &>(*shadowNodeForTextShadowNode), "");
                                                                       ^
xplat/js/react-native-github/ReactCommon/react/renderer/core/tests/traitCastTest.cpp:128:3: error: use of undeclared identifier 'EXPECT_NO_FATAL_FAILURE'
  EXPECT_NO_FATAL_FAILURE(
  ^
xplat/js/react-native-github/ReactCommon/react/renderer/core/tests/traitCastTest.cpp:133:73: error: too few arguments provided to function-like macro invocation
      traitCast<TextShadowNode const &>(*shadowNodeForRawTextShadowNode));
                                                                        ^
xplat/js/react-native-github/ReactCommon/react/renderer/core/tests/traitCastTest.cpp:132:3: error: use of undeclared identifier 'EXPECT_DEATH_IF_SUPPORTED'
  EXPECT_DEATH_IF_SUPPORTED(
  ^
xplat/js/react-native-github/ReactCommon/react/renderer/core/tests/traitCastTest.cpp:137:57: error: too few arguments provided to function-like macro invocation
      traitCast<TextShadowNode const &>(*viewShadowNode));
                                                        ^
xplat/js/react-native-github/ReactCommon/react/renderer/core/tests/traitCastTest.cpp:136:3: error: use of undeclared identifier 'EXPECT_DEATH_IF_SUPPORTED'
  EXPECT_DEATH_IF_SUPPORTED(
  ^
stderr: xplat/js/react-native-github/ReactCommon/react/renderer/core/tests/traitCastTest.cpp:114:78: error: too many arguments provided to function-like macro invocation
      traitCast<RawTextShadowNode const &>(*shadowNodeForRawTextShadowNode), "");
                                                                             ^
xplat/js/react-native-github/ReactCommon/react/renderer/core/tests/traitCastTest.cpp:113:3: error: use of undeclared identifier 'EXPECT_NO_FATAL_FAILURE'
  EXPECT_NO_FATAL_FAILURE(
  ^
xplat/js/react-native-github/ReactCommon/react/renderer/core/tests/traitCastTest.cpp:118:73: error: too few arguments provided to function-like macro invocation
      traitCast<RawTextShadowNode const &>(*shadowNodeForTextShadowNode));
                                                                        ^
xplat/js/react-native-github/ReactCommon/react/renderer/core/tests/traitCastTest.cpp:117:3: error: use of undeclared identifier 'EXPECT_DEATH_IF_SUPPORTED'
  EXPECT_DEATH_IF_SUPPORTED(
  ^
xplat/js/react-native-github/ReactCommon/react/renderer/core/tests/traitCastTest.cpp:122:60: error: too few arguments provided to function-like macro invocation
      traitCast<RawTextShadowNode const &>(*viewShadowNode));
                                                           ^
xplat/js/react-native-github/ReactCommon/react/renderer/core/tests/traitCastTest.cpp:121:3: error: use of undeclared identifier 'EXPECT_DEATH_IF_SUPPORTED'
  EXPECT_DEATH_IF_SUPPORTED(
  ^
xplat/js/react-native-github/ReactCommon/react/renderer/core/tests/traitCastTest.cpp:129:72: error: too many arguments provided to function-like macro invocation
      traitCast<TextShadowNode const &>(*shadowNodeForTextShadowNode), "");
                                                                       ^
xplat/js/react-native-github/ReactCommon/react/renderer/core/tests/traitCastTest.cpp:128:3: error: use of undeclared identifier 'EXPECT_NO_FATAL_FAILURE'
  EXPECT_NO_FATAL_FAILURE(
  ^
xplat/js/react-native-github/ReactCommon/react/renderer/core/tests/traitCastTest.cpp:133:73: error: too few arguments provided to function-like macro invocation
      traitCast<TextShadowNode const &>(*shadowNodeForRawTextShadowNode));
                                                                        ^
xplat/js/react-native-github/ReactCommon/react/renderer/core/tests/traitCastTest.cpp:132:3: error: use of undeclared identifier 'EXPECT_DEATH_IF_SUPPORTED'
  EXPECT_DEATH_IF_SUPPORTED(
  ^
xplat/js/react-native-github/ReactCommon/react/renderer/core/tests/traitCastTest.cpp:137:57: error: too few arguments provided to function-like macro invocation
      traitCast<TextShadowNode const &>(*viewShadowNode));
                                                        ^
xplat/js/react-native-github/ReactCommon/react/renderer/core/tests/traitCastTest.cpp:136:3: error: use of undeclared identifier 'EXPECT_DEATH_IF_SUPPORTED'
  EXPECT_DEATH_IF_SUPPORTED(
  ^
 ** Summary of failures encountered during the build **
Rule //xplat/js/react-native-github/ReactCommon/react/renderer/core:testsApple FAILED because Command failed with exit code 1.
command: [/mnt/btrfs/trunk-hg-fbobjc-fbsource-145066361-1623361918/buck-out/gen/33fbdb84/xplat/toolchains/facebook-dt/tools/pika-11_clang++_noasserts_linux/clang++, -resource-dir, /mnt/btrfs/trunk-hg-fbobjc-fbsource-145066361-1623361918/buck-out/gen/33fbdb84/xplat/toolchains/facebook-dt/tools/pika-11_clang-resource-dir_noassert...
<truncated>
...
stderr: xplat/js/react-native-github/ReactCommon/react/renderer/core/tests/traitCastTest.cpp:114:78: error: too many arguments provided to function-like macro invocation
      traitCast<RawTextShadowNode const &>(*shadowNodeForRawTextShadowNode), "");
                                                                             ^
xplat/third-party/gmock/googletest-1.8.0/googletest/include/gtest/gtest.h:2115:9: note: macro 'EXPECT_NO_FATAL_FAILURE' defined here
#define EXPECT_NO_FATAL_FAILURE(statement) \
        ^

facebook-github-bot · 2021-06-11T02:15:06Z

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

JoshuaGross · 2021-06-11T02:17:08Z

K, trying again to land - if this doesn't work we can iterate again tomorrow.

JoshuaGross · 2021-06-11T02:26:25Z

I think the JS test failing is unrelated... I'll investigate tomorrow

facebook-github-bot · 2021-06-11T18:52:45Z

@JoshuaGross merged this pull request in 050922a.

acoates-ms added 2 commits June 9, 2021 15:00

Remove usages of RTTI in places used by RNW

253aee7

build fixes

624c7c2

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 labels Jun 9, 2021

Add UTs for RawText and Text traitCasts

abde8b7

Had some UT expectations inverted

2edb4d8

Blind coding.. what could go wrong.

1e51d0f

facebook-github-bot closed this in 050922a Jun 11, 2021

facebook-github-bot added the Merged This PR has been merged. label Jun 11, 2021

acoates-ms deleted the nortti branch September 2, 2022 17:32

Conversation

acoates-ms commented Jun 9, 2021

Summary

Changelog

Test Plan

Uh oh!

analysis-bot commented Jun 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

analysis-bot commented Jun 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoshuaGross commented Jun 10, 2021

Uh oh!

sammy-SC commented Jun 10, 2021

Uh oh!

facebook-github-bot commented Jun 10, 2021

Uh oh!

sammy-SC commented Jun 10, 2021

Uh oh!

JoshuaGross commented Jun 10, 2021

Uh oh!

acoates-ms commented Jun 10, 2021

Uh oh!

JoshuaGross commented Jun 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoshuaGross commented Jun 10, 2021

Uh oh!

acoates-ms commented Jun 10, 2021

Uh oh!

facebook-github-bot commented Jun 10, 2021

Uh oh!

JoshuaGross commented Jun 10, 2021

Uh oh!

facebook-github-bot commented Jun 11, 2021

Uh oh!

JoshuaGross commented Jun 11, 2021

Uh oh!

JoshuaGross commented Jun 11, 2021

Uh oh!

facebook-github-bot commented Jun 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

analysis-bot commented Jun 9, 2021 •

edited

Loading

analysis-bot commented Jun 10, 2021 •

edited

Loading

JoshuaGross commented Jun 10, 2021 •

edited

Loading