Skip to content

Commit

Permalink
Prevent type conversion in Differentiator
Browse files Browse the repository at this point in the history 
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, facebook#16                     // =16
str     w0, [sp, facebook#12]
ldr     w8, [sp, facebook#12]
add     w9, w8, #1                      // =1
mov     w8, w9
sxtw    x0, w8
add     sp, sp, facebook#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, facebook#16             // =16
str     x0, [sp, #8]
ldr     x8, [sp, #8]
add     x0, x8, #1              // =1
add     sp, sp, facebook#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
  • Loading branch information
@sammy-SC @facebook-github-bot
sammy-SC authored and facebook-github-bot committed Oct 15, 2020
1 parent b70152c commit 81a97de
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 25 deletions.
48 changes: 24 additions & 24 deletions ReactCommon/react/renderer/mounting/Differentiator.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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 ? "*" : "");
Expand Down Expand Up @@ -410,7 +410,7 @@ static void calculateShadowViewMutationsFlattener(
auto deletionCreationCandidatePairs =
TinyMap<Tag, ShadowViewNodePair const *>{};

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
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -767,7 +767,7 @@ static void calculateShadowViewMutationsV2(
return;
}

auto index = int{0};
size_t index = 0;

// Lists of mutations
auto createMutations = ShadowViewMutation::List{};
Expand All @@ -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(
Expand All @@ -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(
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand All @@ -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({
Expand Down Expand Up @@ -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];
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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];
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand All @@ -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({
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion ReactCommon/react/renderer/mounting/ShadowView.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ struct ShadowViewNodePair final {
bool flattened{false};
bool isConcreteView{true};

int mountIndex{0};
size_t mountIndex{0};

bool inOtherTree{false};

Expand Down

0 comments on commit 81a97de

Please sign in to comment.