Consolidate large record mutation output into a single spread object literal#8473
Consolidate large record mutation output into a single spread object literal#8473cometkim wants to merge 3 commits into
Conversation
…teral
So it emits `{...x, a: 1}` instead of `let newrecord = {...x}; newrecord.a = 1;`.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55281545d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| {primitive = Psetfield (_, fld_info); args = [Lvar id'; value]; _}, | ||
| rest ) | ||
| when Ident.same id' copy_id -> | ||
| collect_dup_overrides copy_id rest ((fld_info, value) :: acc) |
There was a problem hiding this comment.
When a record update overrides more than one field and the override expressions have side effects, this collector reverses the Lsequence order: each Psetfield is consed onto acc, and the fold below compiles that reversed list. The previous output executed the Psetfield chain in order, so cases like two logging/counter updates now run in the opposite order. Collect in order, or reverse before compiling values, before emitting the object literal.
Useful? React with 👍 / 👎.
| Some | ||
| (Js_output.output_of_block_and_expression lambda_cxt.continuation | ||
| blocks | ||
| (E.obj ~dup:init_val (List.rev props)))) |
There was a problem hiding this comment.
When the optimized record update is used only for effects and the override values are pure, this emits a single Object(Some spread, props) value; Js_analyzer.no_side_effect_expression currently treats Object(_, kvs) as pure by checking only kvs, so append_output/output_as_block can drop {...getRecord(), x: 1} entirely and skip the side effects of getRecord(). The old temp-plus-assignment path kept the spread initializer live because the temp was used by the assignment.
Useful? React with 👍 / 👎.
| title={"barry"} | ||
| {...baseProps} | ||
| className={"barry"} | ||
| title={"barry"} |
There was a problem hiding this comment.
The order changed because it now follows the declaration order rather than the source order.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8473 +/- ##
=======================================
Coverage 74.42% 74.42%
=======================================
Files 451 451
Lines 61459 61497 +38
=======================================
+ Hits 45743 45772 +29
- Misses 15716 15725 +9
🚀 New features to boost your workflow:
|
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
cknitt
left a comment
cknitt
left a comment
There was a problem hiding this comment.
Nice improvement! 🎉
Tested against one of our company projects and works fine.
@cristianoc Do you maybe also want to have a look before we merge?
Hmm, ok, question is if we want to do that. |
That is, if we continue our efforts not to change the ml's IR structure. However, we know that someday we will have to fold the two IRs into one. Just put it off until that moment. |
2 participants
Follow-up to #7043
To avoid excessive variations in the Lambda types, I added a kind of pattern matching for record mutation (Pduprecord + Psetfield chain).
But it is not a very efficient implementation; it might be better to just change the IR.