Skip to content
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

[compiler] Wrap inline jsx transform codegen in conditional #31267

Merged
merged 2 commits into from
Nov 4, 2024

Conversation

jackpope
Copy link
Contributor

JSX inlining is a prod-only optimization. We want to enforce this while maintaining the same compiler output in DEV and PROD.

Here we add a conditional to the transform that only replaces JSX with object literals outside of DEV. Then a later build step can handle DCE based on the value of __DEV__

Copy link

vercel bot commented Oct 15, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 28, 2024 3:05pm

@jackpope
Copy link
Contributor Author

@mofeiZ it looks like the fbt/fbt-call-complex-param-value test is failing here with a debug build. This happens to the inline jsx test when I use __DEV__ instead of DEV

@mofeiZ
Copy link
Contributor

mofeiZ commented Oct 17, 2024

@mofeiZ it looks like the fbt/fbt-call-complex-param-value test is failing here with a debug build. This happens to the inline jsx test when I use __DEV__ instead of DEV

Ahh looks like the issue is that the newly added test fixture writes to global.__DEV__. We currently do not run snap fixtures in with an isolated global object/dom, so this also affects compilation (see check for whether we need to insert fast refresh).

Do you need to set the global __DEV__ in this unit test? We probably patch the evaluator to support this if needed

Copy link
Contributor

@josephsavona josephsavona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay on review, see comments

Comment on lines 101 to 115
...currentBlock,
id: fallthroughBlockId,
instructions: fallthroughBlockInstructions,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably safer to reset phis, preds, etc here, or more generally to just copy the specific properties you want to keep (which is really only block.kind)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to do explicit copies. I think we want to reuse the terminal from the current block in addition to the kind since we are splitting here?

Comment on lines +129 to +155
const varInstruction: Instruction = {
id: makeInstructionId(0),
lvalue: {...varLValuePlace},
value: {
kind: 'DeclareLocal',
lvalue: {place: {...varPlace}, kind: InstructionKind.Let},
type: null,
loc: instr.value.loc,
},
loc: instr.loc,
};
currentBlockInstructions.push(varInstruction);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm this isn't safe. There are two main categories of blocks which correspond roughly to statement-level blocks (kind 'block') and expression-level blocks (kind 'logical' etc). It's only safe to add a variable declaration at a block statement. So i think what you'd need to do is write this as a ternary, with an unnamed (non-promoted) variable that is getting assigned one of two different values without a variable declaration.

ie you're doing:

let t0;
if (__DEV__) {
  t0 = { $$typeof: 'react.element', ...}
} else {
  t0 = jsx(...);
}

When it needs to be

__DEV__ ? { $$typeof: ... } : jsx(...)

So that it works in statement and expression contexts.

Ternaries are a bit tedious to create, you need both a ternary terminal and a branch terminal. But you get to avoid the variable declaration. You'll still need to create the phi in the fallthrough block.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to skip value blocks with a TODO test and a diagnostic log

@josephsavona
Copy link
Contributor

A failing example would be something like:

function Component({cond}) {
  return cond ? <div /> : null;
}

because we'll try to create a variable declaration inside the consequent of the ternary, but that's a statement where only expressions are allowed.

@josephsavona
Copy link
Contributor

Chatted with @mofeiZ about this briefly — she suggested that we could unblock getting some experimental results by just skipping JSX within value blocks. That seems like a great compromise, and if the data is promising we can come back to handle value blocks.

@jackpope jackpope force-pushed the conditional-jsx-inline branch from c043c1e to 8bb3ca3 Compare October 28, 2024 14:59
Copy link
Contributor

@josephsavona josephsavona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome!

@jackpope jackpope merged commit 543eb09 into facebook:main Nov 4, 2024
19 checks passed
@jackpope jackpope deleted the conditional-jsx-inline branch November 4, 2024 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants