New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[hack pipes] Inline topic token when possible #14278
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,27 @@ | ||
import { types as t } from "@babel/core"; | ||
import type { NodePath, Visitor } from "@babel/traverse"; | ||
|
||
const topicReferenceReplacementVisitor = { | ||
TopicReference(path) { | ||
path.replaceWith(t.cloneNode(this.topicVariable)); | ||
const topicReferenceVisitor: Visitor<{ | ||
topicReferences: NodePath<t.TopicReference>[]; | ||
sideEffectsBeforeFirstTopicReference: boolean; | ||
}> = { | ||
exit(path, state) { | ||
if (path.isTopicReference()) { | ||
state.topicReferences.push(path); | ||
} else { | ||
if ( | ||
state.topicReferences.length === 0 && | ||
!state.sideEffectsBeforeFirstTopicReference && | ||
!path.isPure() | ||
) { | ||
state.sideEffectsBeforeFirstTopicReference = true; | ||
} | ||
} | ||
}, | ||
"ClassBody|Function"(_, state) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Class body does not encapsulate all expressions under class, for example: class decorators. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's exactly why I used ClassBody: class decorators evaluation is not deferred! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If so then we have to exclude computed class element keys, too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem is that exactly determining what runs "now" is hard. The list includes (but is not limited to):
To avoid making the transform too complex, I'd prefer to leave it as-is and deoptimize when they happen. |
||
if (state.topicReferences.length === 0) { | ||
state.sideEffectsBeforeFirstTopicReference = true; | ||
} | ||
}, | ||
}; | ||
|
||
|
@@ -22,22 +41,41 @@ export default { | |
return; | ||
} | ||
|
||
const topicVariable = scope.generateUidIdentifierBasedOnNode(node); | ||
const pipeBodyPath = path.get("right"); | ||
|
||
scope.push({ id: topicVariable }); | ||
|
||
if (pipeBodyPath.node.type === "TopicReference") { | ||
// If the pipe body is itself a lone topic reference, | ||
// then replace it with the topic variable. | ||
pipeBodyPath.replaceWith(t.cloneNode(topicVariable)); | ||
} else { | ||
// Replace topic references with the topic variable. | ||
pipeBodyPath.traverse(topicReferenceReplacementVisitor, { | ||
topicVariable, | ||
}); | ||
// then replace the whole expression with its left operand. | ||
path.replaceWith(node.left); | ||
return; | ||
} | ||
|
||
const visitorState = { | ||
topicReferences: [], | ||
// pipeBodyPath might be a function, and it won't be visited by | ||
// topicReferenceVisitor because traverse() skips the top-level | ||
// node. We must handle that case here manually. | ||
sideEffectsBeforeFirstTopicReference: pipeBodyPath.isFunction(), | ||
}; | ||
pipeBodyPath.traverse(topicReferenceVisitor, visitorState); | ||
|
||
if ( | ||
visitorState.topicReferences.length === 1 && | ||
(!visitorState.sideEffectsBeforeFirstTopicReference || | ||
path.scope.isPure(node.left, true)) | ||
) { | ||
visitorState.topicReferences[0].replaceWith(node.left); | ||
path.replaceWith(node.right); | ||
return; | ||
} | ||
|
||
const topicVariable = scope.generateUidIdentifierBasedOnNode(node); | ||
scope.push({ id: topicVariable }); | ||
|
||
// Replace topic references with the topic variable. | ||
visitorState.topicReferences.forEach(path => | ||
path.replaceWith(t.cloneNode(topicVariable)), | ||
); | ||
|
||
// Replace the pipe expression itself with an assignment expression. | ||
path.replaceWith( | ||
t.sequenceExpression([ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,2 @@ | ||
var _ref; | ||
|
||
Tuple(0); | ||
_ref = 1, Tuple(0, _ref); | ||
Tuple(0, 1); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,2 @@ | ||
var _ref; | ||
|
||
const result = (_ref = 5, _ref); | ||
const result = 5; | ||
expect(result).toBe(5); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,3 @@ | ||
const result = 5 |> ^ + 1 |> ^ + ^; | ||
const result = 5 |> ^ + 1 |> 2 + ^ |> ^ + ^; | ||
|
||
expect(result).toBe(12); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
var _ref, _ref2; | ||
var _ref; | ||
|
||
const result = (_ref2 = 5, (_ref = _ref2 + 1, _ref + _ref)); | ||
const result = (_ref = 2 + (5 + 1), _ref + _ref); | ||
expect(result).toBe(12); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,8 @@ | ||
var _ref4, _ref5, _ref6; | ||
var _ref2; | ||
|
||
const result = (_ref6 = 5, (_ref5 = Math.pow(_ref6, 2), (_ref4 = [1, 2, 3].map(n => { | ||
var _ref, _ref2, _ref3; | ||
const result = (_ref2 = Math.pow(5, 2), [1, 2, 3].map(n => { | ||
var _ref; | ||
|
||
return _ref3 = n + _ref5, (_ref2 = _ref3 * 2, (_ref = `${_ref2} apples`, _ref.toUpperCase())); | ||
}), _ref4.join()))); | ||
return _ref = (n + _ref2) * 2, `${_ref} apples`.toUpperCase(); | ||
}).join()); | ||
expect(result).toEqual('52 APPLES,54 APPLES,56 APPLES'); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,8 @@ | ||
var _ref, _ref2, _ref3; | ||
var _ref; | ||
|
||
const result = (_ref3 = -2.2 // -2.2 | ||
, (_ref2 = Math.floor(_ref3) // -3 | ||
, (_ref = () => Math.pow(_ref2, 5) // () => -243 | ||
, _ref()))); // -243 | ||
const result = (_ref = Math.floor(-2.2 // -2.2 | ||
) // -3 | ||
, (() => Math.pow(_ref, 5) // () => -243 | ||
)()); // -243 | ||
|
||
expect(result).toBe(-243); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,3 @@ | ||
var _ref; | ||
|
||
const program = '(function() { return this; })()'; | ||
const result = (_ref = program, eval(_ref)); | ||
const result = eval(program); | ||
expect(result).not.toBeUndefined(); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
var _ref, _ref2, _ref3, _ref4; | ||
var _ref; | ||
|
||
const result = (_ref4 = 5, (_ref3 = Math.pow(_ref4, 2), (_ref2 = _ref3 + 1, (_ref = `${_ref2} apples`, _ref.toUpperCase())))); | ||
const result = (_ref = Math.pow(5, 2) + 1, `${_ref} apples`.toUpperCase()); | ||
expect(result).toEqual('26 APPLES'); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,11 @@ | ||
var _ref, _ref2, _ref3; | ||
var _ref; | ||
|
||
function area(rect) { | ||
return rect.width * rect.height; | ||
} | ||
|
||
const result = (_ref3 = -5, (_ref2 = Math.abs(_ref3), (_ref = { | ||
width: _ref2, | ||
height: _ref2 + 3 | ||
}, area(_ref)))); | ||
const result = (_ref = Math.abs(-5), area({ | ||
width: _ref, | ||
height: _ref + 3 | ||
})); | ||
expect(result).toBe(40); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,2 @@ | ||
var _ref; | ||
|
||
const result = (_ref = 'Hello', _ref.toUpperCase()); | ||
const result = 'Hello'.toUpperCase(); | ||
expect(result).toBe('HELLO'); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,2 @@ | ||
var _ref, _ref2; | ||
|
||
const result = (_ref2 = (_ref = 5, Math.pow(_ref, 2)), _ref2 + 1); | ||
const result = Math.pow(5, 2) + 1; | ||
expect(result).toEqual(26); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
let x = 0; | ||
|
||
let fnA = x++ |> (() => ^); | ||
let fnB = x++ |> (0, () => ^); | ||
|
||
expect(x).toBe(2); | ||
expect(fnA()).toBe(0); | ||
expect(fnB()).toBe(1); | ||
expect(x).toBe(2); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
let x = 0; | ||
|
||
let fnA = x++ |> (() => ^); | ||
let fnB = x++ |> (0, () => ^); | ||
|
||
expect(x).toBe(2); | ||
expect(fnA()).toBe(0); | ||
expect(fnB()).toBe(1); | ||
expect(x).toBe(2); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
var _ref, _ref2; | ||
|
||
let x = 0; | ||
let fnA = (_ref = x++, () => _ref); | ||
let fnB = (_ref2 = x++, (0, () => _ref2)); | ||
expect(x).toBe(2); | ||
expect(fnA()).toBe(0); | ||
expect(fnB()).toBe(1); | ||
expect(x).toBe(2); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,6 @@ | ||
var _ref, _ref2; | ||
|
||
const triple = function (x) { | ||
return x * 3; | ||
}; | ||
|
||
const result = (_ref2 = -7, (_ref = Math.abs(_ref2), triple(_ref))); | ||
const result = triple(Math.abs(-7)); | ||
return expect(result).toBe(21); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,9 +3,9 @@ const triple = function (x) { | |
}; | ||
|
||
async function myFunction(n) { | ||
var _ref, _ref2, _ref3, _ref4; | ||
var _ref; | ||
|
||
return _ref4 = n, (_ref3 = Math.abs(_ref4), (_ref2 = Promise.resolve(_ref3), (_ref = await _ref2, triple(_ref)))); | ||
return _ref = Math.abs(n), triple(await Promise.resolve(_ref)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't fully inlined because return triple(await Promise.resolve(Math.abs(n))); |
||
} | ||
|
||
return myFunction(-7).then(function (result) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
let i = 0; | ||
let sum = 0; | ||
|
||
while (i |> (i = ^ + 1) |> ^ <= 10) | ||
while (i |> (i = 2 * ^ - ^ + 1) |> ^ <= 10) | ||
sum += i; | ||
|
||
expect(sum).toBe(10 + 9 + 8 + 7 + 6 + 5 + 4 + 3 + 2 + 1); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
let i = 0; | ||
let sum = 0; | ||
|
||
while (i |> (i = ^ + 1) |> ^ <= 10) | ||
while (i |> (i = 2 * ^ - ^ + 1) |> ^ <= 10) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is just to keep the "spirit" of the test, otherwise it was completely inlined. |
||
sum += i; | ||
|
||
expect(sum).toBe(10 + 9 + 8 + 7 + 6 + 5 + 4 + 3 + 2 + 1); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,2 @@ | ||
var _ref; | ||
|
||
const x = (_ref = 0, _ref + 1); | ||
const x = 0 + 1; | ||
expect(x).toBe(1); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
let sum = 0; | ||
for (var i = 0 |> ^; i <= 10; i++) | ||
for (var i = 0 |> ^*^; i <= 10; i++) | ||
sum += i; | ||
|
||
expect(sum).toBe(10 + 9 + 8 + 7 + 6 + 5 + 4 + 3 + 2 + 1); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
let sum = 0; | ||
for (var i = 0; i |> ^ <= 10; i++) | ||
for (var i = 0; i |> ^ + ^ <= 20; i++) | ||
sum = sum + i; | ||
|
||
expect(sum).toBe(10 + 9 + 8 + 7 + 6 + 5 + 4 + 3 + 2 + 1) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
let sum = 0; | ||
for (var i = 0; i |> ^ <= 10; i++) | ||
for (var i = 0; i |> ^ + ^ <= 20; i++) | ||
sum = sum + i; | ||
|
||
expect(sum).toBe(10 + 9 + 8 + 7 + 6 + 5 + 4 + 3 + 2 + 1) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
function* myGenerator(n) { | ||
var _ref, _ref2; | ||
var _ref; | ||
|
||
return _ref2 = n, (_ref = yield _ref2, Math.abs(_ref)); | ||
return _ref = yield n, Math.abs(_ref); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is sideEffectsBeforeFirstTopicReference invalidated only when side effects occur before the first topic reference in an RHS? For example, will it be set to true for
x |> (f(x), g(x))
x |> (f(#), g(#))
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you mean
x |> (f(#), g(#))
and thatf
/g
are defined and not reassigned. It will not be set to true because there are no side effects before the first#
. However, since there are multiple#
s we won't inline anything.I only tracked the side effects before the first one, so that
x |> (#, SIDE_EFFECT)
doesn't prevent inlining.