From 81a97de546daf9a23220cd7c1d28032cf2cb0b92 Mon Sep 17 00:00:00 2001 From: Samuel Susla Date: Thu, 15 Oct 2020 15:11:16 -0700 Subject: [PATCH] Prevent type conversion in Differentiator Summary: changelog: [internal] Prevents 2 type converions: 1. int <-> size_t 2. int <-> int32_t # Why is using size_t better when working with indexes. ## 1. Type conversion isn't for free. Take this example ``` size_t calculate(int number) { return number + 1; } ``` It generates following assembly (generated with armv8-a clang 10.0.0): ``` calculate(int): // calculate(int) sub sp, sp, #16 // =16 str w0, [sp, #12] ldr w8, [sp, #12] add w9, w8, #1 // =1 mov w8, w9 sxtw x0, w8 add sp, sp, #16 // =16 ret ``` That's 9 instructions. If we get rid of type conversion: ``` size_t calculate(size_t number) { return number + 1; } ``` Assembly (generated with armv8-a clang 10.0.0): ``` calculate(unsigned long): // calculate(unsigned long) sub sp, sp, #16 // =16 str x0, [sp, #8] ldr x8, [sp, #8] add x0, x8, #1 // =1 add sp, sp, #16 // =16 ret ``` Compiler now produces only 7 instructions. ## Semantics When using int for indexing, the type doesn't say much. By using `size_t`, just by looking at the type, it gives the reader more information about where it is coming from. Reviewed By: JoshuaGross Differential Revision: D24332248 fbshipit-source-id: 87ef982829ec14906ed9e002ea2e875fda4a0cd8 --- .../renderer/mounting/Differentiator.cpp | 48 +++++++++---------- .../react/renderer/mounting/ShadowView.h | 2 +- 2 files changed, 25 insertions(+), 25 deletions(-) diff --git a/ReactCommon/react/renderer/mounting/Differentiator.cpp b/ReactCommon/react/renderer/mounting/Differentiator.cpp index d7fee08924385b..1bf8fe6e40d8fe 100644 --- a/ReactCommon/react/renderer/mounting/Differentiator.cpp +++ b/ReactCommon/react/renderer/mounting/Differentiator.cpp @@ -251,7 +251,7 @@ ShadowViewNodePair::List sliceChildShadowNodeViewPairsV2( reorderInPlaceIfNeeded(pairList); // Set list and mountIndex for each after reordering - int mountIndex = 0; + size_t mountIndex = 0; for (auto &child : pairList) { child.mountIndex = (child.isConcreteView ? mountIndex++ : -1); } @@ -376,7 +376,7 @@ static void calculateShadowViewMutationsFlattener( << " [" << node.shadowView.tag << "]"; LOG(ERROR) << "Differ Flattener Entry: Child Pairs: "; std::string strTreeChildPairs; - for (int k = 0; k < treeChildren.size(); k++) { + for (size_t k = 0; k < treeChildren.size(); k++) { strTreeChildPairs.append(std::to_string(treeChildren[k].shadowView.tag)); strTreeChildPairs.append(treeChildren[k].isConcreteView ? "" : "'"); strTreeChildPairs.append(treeChildren[k].flattened ? "*" : ""); @@ -410,7 +410,7 @@ static void calculateShadowViewMutationsFlattener( auto deletionCreationCandidatePairs = TinyMap{}; - for (int index = 0; + for (size_t index = 0; index < treeChildren.size() && index < treeChildren.size(); index++) { // First, remove all children of the tree being flattened, or insert @@ -530,7 +530,7 @@ static void calculateShadowViewMutationsFlattener( // this "else" block, including any annotations we put on them. auto newFlattenedNodes = sliceChildShadowNodeViewPairsV2( *newTreeNodePair.shadowNode, true); - for (int i = 0; i < newFlattenedNodes.size(); i++) { + for (size_t i = 0; i < newFlattenedNodes.size(); i++) { auto &newChild = newFlattenedNodes[i]; auto unvisitedOtherNodesIt = @@ -582,7 +582,7 @@ static void calculateShadowViewMutationsFlattener( // this "else" block, including any annotations we put on them. auto oldFlattenedNodes = sliceChildShadowNodeViewPairsV2( *oldTreeNodePair.shadowNode, true); - for (int i = 0; i < oldFlattenedNodes.size(); i++) { + for (size_t i = 0; i < oldFlattenedNodes.size(); i++) { auto &oldChild = oldFlattenedNodes[i]; auto unvisitedOtherNodesIt = @@ -767,7 +767,7 @@ static void calculateShadowViewMutationsV2( return; } - auto index = int{0}; + size_t index = 0; // Lists of mutations auto createMutations = ShadowViewMutation::List{}; @@ -790,7 +790,7 @@ static void calculateShadowViewMutationsV2( LOG(ERROR) << "Differ Entry: Child Pairs of node: [" << parentShadowView.tag << "]"; std::string strOldChildPairs; - for (int oldIndex = 0; oldIndex < oldChildPairs.size(); oldIndex++) { + for (size_t oldIndex = 0; oldIndex < oldChildPairs.size(); oldIndex++) { strOldChildPairs.append( std::to_string(oldChildPairs[oldIndex].shadowView.tag)); strOldChildPairs.append( @@ -799,7 +799,7 @@ static void calculateShadowViewMutationsV2( strOldChildPairs.append(", "); } std::string strNewChildPairs; - for (int newIndex = 0; newIndex < newChildPairs.size(); newIndex++) { + for (size_t newIndex = 0; newIndex < newChildPairs.size(); newIndex++) { strNewChildPairs.append( std::to_string(newChildPairs[newIndex].shadowView.tag)); strNewChildPairs.append( @@ -872,7 +872,7 @@ static void calculateShadowViewMutationsV2( } } - int lastIndexAfterFirstStage = index; + size_t lastIndexAfterFirstStage = index; if (index == newChildPairs.size()) { // We've reached the end of the new children. We can delete+remove the @@ -943,9 +943,9 @@ static void calculateShadowViewMutationsV2( // Walk through both lists at the same time // We will perform updates, create+insert, remove+delete, remove+insert // (move) here. - int oldIndex = lastIndexAfterFirstStage, - newIndex = lastIndexAfterFirstStage, newSize = newChildPairs.size(), - oldSize = oldChildPairs.size(); + size_t oldIndex = lastIndexAfterFirstStage, + newIndex = lastIndexAfterFirstStage, newSize = newChildPairs.size(), + oldSize = oldChildPairs.size(); while (newIndex < newSize || oldIndex < oldSize) { bool haveNewPair = newIndex < newSize; bool haveOldPair = oldIndex < oldSize; @@ -955,8 +955,8 @@ static void calculateShadowViewMutationsV2( auto const &oldChildPair = oldChildPairs[oldIndex]; auto const &newChildPair = newChildPairs[newIndex]; - int newTag = newChildPair.shadowView.tag; - int oldTag = oldChildPair.shadowView.tag; + Tag newTag = newChildPair.shadowView.tag; + Tag oldTag = oldChildPair.shadowView.tag; if (newTag == oldTag) { DEBUG_LOGS({ @@ -1050,7 +1050,7 @@ static void calculateShadowViewMutationsV2( // children from other nodes, etc. auto oldFlattenedNodes = sliceChildShadowNodeViewPairsV2( *oldChildPair.shadowNode, true); - for (int i = 0, j = 0; + for (size_t i = 0, j = 0; i < oldChildPairs.size() && j < oldFlattenedNodes.size(); i++) { auto &oldChild = oldChildPairs[i]; @@ -1119,7 +1119,7 @@ static void calculateShadowViewMutationsV2( if (haveOldPair) { auto const &oldChildPair = oldChildPairs[oldIndex]; - int oldTag = oldChildPair.shadowView.tag; + Tag oldTag = oldChildPair.shadowView.tag; // Was oldTag already inserted? This indicates a reordering, not just // a move. The new node has already been inserted, we just need to @@ -1171,7 +1171,7 @@ static void calculateShadowViewMutationsV2( // children from other nodes, etc. auto oldFlattenedNodes = sliceChildShadowNodeViewPairsV2( *oldChildPair.shadowNode, true); - for (int i = 0, j = 0; + for (size_t i = 0, j = 0; i < oldChildPairs.size() && j < oldFlattenedNodes.size(); i++) { auto &oldChild = oldChildPairs[i]; @@ -1565,7 +1565,7 @@ static void calculateShadowViewMutations( std::move(newGrandChildPairs)); } - int lastIndexAfterFirstStage = index; + size_t lastIndexAfterFirstStage = index; if (index == newChildPairs.size()) { // We've reached the end of the new children. We can delete+remove the @@ -1630,9 +1630,9 @@ static void calculateShadowViewMutations( // Walk through both lists at the same time // We will perform updates, create+insert, remove+delete, remove+insert // (move) here. - int oldIndex = lastIndexAfterFirstStage, - newIndex = lastIndexAfterFirstStage, newSize = newChildPairs.size(), - oldSize = oldChildPairs.size(); + size_t oldIndex = lastIndexAfterFirstStage, + newIndex = lastIndexAfterFirstStage, newSize = newChildPairs.size(), + oldSize = oldChildPairs.size(); while (newIndex < newSize || oldIndex < oldSize) { bool haveNewPair = newIndex < newSize; bool haveOldPair = oldIndex < oldSize; @@ -1642,8 +1642,8 @@ static void calculateShadowViewMutations( auto const &newChildPair = newChildPairs[newIndex]; auto const &oldChildPair = oldChildPairs[oldIndex]; - int newTag = newChildPair.shadowView.tag; - int oldTag = oldChildPair.shadowView.tag; + Tag newTag = newChildPair.shadowView.tag; + Tag oldTag = oldChildPair.shadowView.tag; if (newTag == oldTag) { DEBUG_LOGS({ @@ -1690,7 +1690,7 @@ static void calculateShadowViewMutations( if (haveOldPair) { auto const &oldChildPair = oldChildPairs[oldIndex]; - int oldTag = oldChildPair.shadowView.tag; + Tag oldTag = oldChildPair.shadowView.tag; // Was oldTag already inserted? This indicates a reordering, not just // a move. The new node has already been inserted, we just need to diff --git a/ReactCommon/react/renderer/mounting/ShadowView.h b/ReactCommon/react/renderer/mounting/ShadowView.h index a481941d7eba2f..3e65e0262c56ff 100644 --- a/ReactCommon/react/renderer/mounting/ShadowView.h +++ b/ReactCommon/react/renderer/mounting/ShadowView.h @@ -70,7 +70,7 @@ struct ShadowViewNodePair final { bool flattened{false}; bool isConcreteView{true}; - int mountIndex{0}; + size_t mountIndex{0}; bool inOtherTree{false};