Skip to content

Commit

Permalink
Get rid of only-value-capture optimization that breaks the world (sof…
Browse files Browse the repository at this point in the history 
…tware-mansion#4060)

## Summary

This PR removes a plugin-level optimization that made it so that when
using nested variables we'd only capture the most inner value and not
the whole object.

The reason is that we now allow objects to be proxies in which case this
won't work as we'd capture the object field instead of capturing the
reference to proxy. An example of such was reported in software-mansion#3967 but we also
noticed similar problems when passing host objects.

We decided to drop optimization that landed in software-mansion#1174 arguing that shared
values after the rewrite are much faster and more reliable hence we can
allow for larger objects to be converted even if we only use one or two
fields from such objects. In the majority of the cases such conversion
will only happen at most once. If this turns out to be problematic in
certain use-cases, the individual fields can be extracted by the user
before the worklet is defined.

In order to make things like Animated module capturable we had to add a
dummy implementation for handling symbols – this isn't used anywhere so
the dummy implementation just converts symbols into strings for now.
Ultimately, symbols would need to have their own registry in order to
guarantee referential equality. But since that'd require adding more
complex logic and is not going to be used for now I decided to go with
that dummy implementation for now instead.

## Test plan
Run regular set of examples. Test software-mansion#1051 and verify things like Animated
object can be captured (see software-mansion#1132 – but this example is no longer valid
as Animated does not have Extrapolate field any longer).
  • Loading branch information
@kmagiera
kmagiera authored Feb 17, 2023
1 parent 5bcc258 commit cf447d8
Show file tree
Hide file tree
Showing 4 changed files with 19 additions and 178 deletions.
9 changes: 8 additions & 1 deletion Common/cpp/NativeModules/NativeReanimatedModule.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ jsi::Value NativeReanimatedModule::makeShareableClone(
}
}
} else if (value.isString()) {
shareable = std::make_shared<ShareableString>(rt, value.asString(rt));
shareable = std::make_shared<ShareableString>(value.asString(rt).utf8(rt));
} else if (value.isUndefined()) {
shareable = std::make_shared<ShareableScalar>();
} else if (value.isNull()) {
Expand All @@ -284,6 +284,13 @@ jsi::Value NativeReanimatedModule::makeShareableClone(
shareable = std::make_shared<ShareableScalar>(value.getBool());
} else if (value.isNumber()) {
shareable = std::make_shared<ShareableScalar>(value.getNumber());
} else if (value.isSymbol()) {
// TODO: this is only a placeholder implementation, here we replace symbols
// with strings in order to make certain objects to be captured. There isn't
// yet any usecase for using symbols on the UI runtime so it is fine to keep
// it like this for now.
shareable =
std::make_shared<ShareableString>(value.getSymbol(rt).toString(rt));
} else {
throw std::runtime_error("attempted to convert an unsupported value type");
}
Expand Down
10 changes: 4 additions & 6 deletions Common/cpp/SharedItems/Shareables.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -427,16 +427,14 @@ class ShareableSynchronizedDataHolder

class ShareableString : public Shareable {
public:
ShareableString(jsi::Runtime &rt, const jsi::String &string)
: Shareable(StringType) {
data = string.utf8(rt);
}
explicit ShareableString(const std::string &string)
: Shareable(StringType), data_(string) {}
jsi::Value toJSValue(jsi::Runtime &rt) override {
return jsi::String::createFromUtf8(rt, data);
return jsi::String::createFromUtf8(rt, data_);
}

protected:
std::string data;
std::string data_;
};

class ShareableScalar : public Shareable {
Expand Down
8 changes: 2 additions & 6 deletions __tests__/__snapshots__/plugin.test.js.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,7 @@ var f = function () {
};
_f._closure = {
x: x,
objX: {
x: objX.x
}
objX: objX
};
_f.__initData = _worklet_5359970077727_init_data;
_f.__workletHash = 5359970077727;
Expand Down Expand Up @@ -157,9 +155,7 @@ function App() {
var a = obj.a;
};
_f._closure = {
obj: {
a: obj.a
}
obj: obj
};
_f.__initData = _worklet_16676723780973_init_data;
_f.__workletHash = 16676723780973;
Expand Down
170 changes: 5 additions & 165 deletions plugin.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,62 +92,6 @@ const globals = new Set([
'_notifyAboutEnd',
]);

// leaving way to avoid deep capturing by adding 'stopCapturing' to the blacklist
const blacklistedFunctions = new Set([
'stopCapturing',
'toString',
'map',
'filter',
'findIndex',
'forEach',
'valueOf',
'toPrecision',
'toExponential',
'constructor',
'toFixed',
'toLocaleString',
'toSource',
'charAt',
'charCodeAt',
'concat',
'indexOf',
'lastIndexOf',
'localeCompare',
'length',
'match',
'replace',
'search',
'slice',
'split',
'substr',
'substring',
'toLocaleLowerCase',
'toLocaleUpperCase',
'toLowerCase',
'toUpperCase',
'every',
'join',
'pop',
'push',
'reduce',
'reduceRight',
'reverse',
'shift',
'slice',
'some',
'sort',
'splice',
'unshift',
'hasOwnProperty',
'isPrototypeOf',
'propertyIsEnumerable',
'bind',
'apply',
'call',
'__callAsync',
'includes',
]);

const gestureHandlerGestureObjects = new Set([
// from https://github.com/software-mansion/react-native-gesture-handler/blob/new-api/src/handlers/gestures/gestureObjects.ts
'Tap',
Expand Down Expand Up @@ -177,112 +121,6 @@ const gestureHandlerBuilderMethods = new Set([
'onTouchesCancelled',
]);

class ClosureGenerator {
constructor() {
this.trie = [{}, false];
}

mergeAns(oldAns, newAns) {
const [purePath, node] = oldAns;
const [purePathUp, nodeUp] = newAns;
if (purePathUp.length !== 0) {
return [purePath.concat(purePathUp), nodeUp];
} else {
return [purePath, node];
}
}

findPrefixRec(path) {
const notFound = [[], null];
if (!path || path.node.type !== 'MemberExpression') {
return notFound;
}
const memberExpressionNode = path.node;
if (memberExpressionNode.property.type !== 'Identifier') {
return notFound;
}
if (
memberExpressionNode.computed ||
memberExpressionNode.property.name === 'value' ||
blacklistedFunctions.has(memberExpressionNode.property.name)
) {
// a.b[w] -> a.b.w in babel nodes
// a.v.value
// sth.map(() => )
return notFound;
}
if (
path.parent &&
path.parent.type === 'AssignmentExpression' &&
path.parent.left === path.node
) {
/// captured.newProp = 5;
return notFound;
}
const purePath = [memberExpressionNode.property.name];
const node = memberExpressionNode;
const upAns = this.findPrefixRec(path.parentPath);
return this.mergeAns([purePath, node], upAns);
}

findPrefix(base, babelPath) {
const purePath = [base];
const node = babelPath.node;
const upAns = this.findPrefixRec(babelPath.parentPath);
return this.mergeAns([purePath, node], upAns);
}

addPath(base, babelPath) {
const [purePath, node] = this.findPrefix(base, babelPath);
let parent = this.trie;
let index = -1;
for (const current of purePath) {
index++;
if (parent[1]) {
continue;
}
if (!parent[0][current]) {
parent[0][current] = [{}, false];
}
if (index === purePath.length - 1) {
parent[0][current] = [node, true];
}
parent = parent[0][current];
}
}

generateNodeForBase(t, current, parent) {
const currentNode = parent[0][current];
if (currentNode[1]) {
return currentNode[0];
}
return t.objectExpression(
Object.keys(currentNode[0]).map((propertyName) =>
t.objectProperty(
t.identifier(propertyName),
this.generateNodeForBase(t, propertyName, currentNode),
false,
true
)
)
);
}

generate(t, variables, names) {
const arrayOfKeys = [...names];
return t.objectExpression(
variables.map((variable, index) =>
t.objectProperty(
t.identifier(variable.name),
this.generateNodeForBase(t, arrayOfKeys[index], this.trie),
false,
true
)
)
);
}
}

function isRelease() {
return ['production', 'release'].includes(process.env.BABEL_ENV);
}
Expand Down Expand Up @@ -426,7 +264,6 @@ function makeWorklet(t, fun, state) {
const functionName = makeWorkletName(t, fun);

const closure = new Map();
const closureGenerator = new ClosureGenerator();

// remove 'worklet'; directive before generating string
fun.traverse({
Expand Down Expand Up @@ -502,7 +339,6 @@ function makeWorklet(t, fun, state) {
currentScope = currentScope.parent;
}
closure.set(name, path.node);
closureGenerator.addPath(name, path);
},
});

Expand Down Expand Up @@ -579,7 +415,11 @@ function makeWorklet(t, fun, state) {
t.assignmentExpression(
'=',
t.memberExpression(privateFunctionId, t.identifier('_closure'), false),
closureGenerator.generate(t, variables, closure.keys())
t.objectExpression(
variables.map((variable) =>
t.objectProperty(t.identifier(variable.name), variable, false, true)
)
)
)
),
t.expressionStatement(
Expand Down

0 comments on commit cf447d8

Please sign in to comment.