Ensure Object.assign merge optimization properly accounts for prefix args #2261
Conversation
src/utils/generator.js
Outdated
| @@ -1367,6 +1367,11 @@ export function attemptToMergeEquivalentObjectAssigns( | |||
| let otherArgsToUse = []; | |||
| for (let x = 1; x < otherArgs.length; x++) { | |||
| let arg = otherArgs[x]; | |||
| // If this arg is a known arg of the root, then we can't merge these Object.assigns | |||
| // as they share the same args. | |||
| if (args.includes(arg)) { | |||
There was a problem hiding this comment.
I actually first opted to add a Set and do has lookups, but found that it was actually much slower here because the arrays in Object.assign are almost always small arrays with generally less than 8 items.
|
Is this a bug fix or an optimization of sorts? I don't quite understand the issue. |
|
@NTillmann It's a bug fix, the test case added fails without this fix. |
| inspect = function() { | ||
| return fn( | ||
| { | ||
| cond1: false, |
There was a problem hiding this comment.
cond1 and cond2 are dead?
| } | ||
|
|
||
| function fn(obj, x) { | ||
| var a = fn2(obj, x); |
There was a problem hiding this comment.
fn2 and fn3 are just getting executed and could be inlined, or are they needed for the regression test?
|
I don't get from the test case right away what the issue is. I could try to debug what went wrong, or can you explain me at a high level what the issue really was? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
trueadm
changed the title
|
@NTillmann As per our conversation, I've correctly fixed this issue as well as the other issue you came up with. We use a different, far more sensical strategy now that involves putting things into "suffixes" and "prefixes" around the "to" we're attempting to merge. |
src/utils/generator.js
Outdated
| if (arg !== possibleOtherObjectAssignTo) { | ||
| newArgs.push(arg); | ||
| } | ||
| // our merged Object.assign, shoud look like: |
There was a problem hiding this comment.
@trueadm is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Release notes: none
When we check to do the temporal
Object.assignmerge optimization, we should properly account for any "prefix" args that occur before "to" and the "suffix" args. Test case added that came up in testing.