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

Change ReactChildrenMutationWarningHook to Object.freeze children #7455

Merged
merged 1 commit into from Sep 13, 2016

Conversation

Projects
None yet
8 participants
@keyanzhang
Member

keyanzhang commented Aug 10, 2016

Alternative of #7410 that fixes #7406. This should also fix #7424 and #7426 (the false-positive warning).
Saving and comparing the shadowChildren is pretty expensive -- we can Object.freeze the JSX children instead.

Benchmark:

Comparing master.txt (control) vs freeze.txt (test)
Significant differences marked by ***
% change from control to test, with 99% CIs:

* factory_ms_jsc_jit
    % change:  +0.38% [ -6.59%,  +7.36%]
    means: 27.6427 (control), 27.7558 (test)
* factory_ms_jsc_nojit
    % change: -10.22% [-15.03%,  -5.41%]  ***
    means: 26.3667 (control), 23.6813 (test)
* factory_ms_node
    % change:  -9.60% [-13.73%,  -5.46%]  ***
    means: 98.522 (control), 89.092 (test)
* ssr_pe_cold_ms_jsc_jit
    % change:  -8.47% [-11.57%,  -5.37%]  ***
    means: 173.219 (control), 158.576 (test)
* ssr_pe_cold_ms_jsc_nojit
    % change: -12.49% [-16.45%,  -8.53%]  ***
    means: 210.597 (control), 184.345 (test)
* ssr_pe_cold_ms_node
    % change:  -7.61% [-10.66%,  -4.56%]  ***
    means: 219.05 (control), 202.417 (test)

Reviewers:
@jimfb @vjeux

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 10, 2016

Member

Is this incremental improvement? If so, can I merge #7410 until this gets reviewed?

Member

gaearon commented Aug 10, 2016

Is this incremental improvement? If so, can I merge #7410 until this gets reviewed?

@jimfb

This comment has been minimized.

Show comment
Hide comment
@jimfb

jimfb Aug 10, 2016

Contributor

At a high level, this looks fine to me. I flagged the slice as a performance note for Sebastian. It would suck if we regress the prod build performance because we were trying to improve dev build performance. If Sebastian signs off on the extra allocation/copy, I have no objections to this diff ( 👍 ).

Contributor

jimfb commented Aug 10, 2016

At a high level, this looks fine to me. I flagged the slice as a performance note for Sebastian. It would suck if we regress the prod build performance because we were trying to improve dev build performance. If Sebastian signs off on the extra allocation/copy, I have no objections to this diff ( 👍 ).

@ghost ghost added the CLA Signed label Aug 10, 2016

@Montana

This comment has been minimized.

Show comment
Hide comment
@Montana

Montana Aug 11, 2016

So we can't merge #7410?

Montana commented Aug 11, 2016

So we can't merge #7410?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 11, 2016

Member

I already merged #7410 in the meantime but this might be a better solution.

Member

gaearon commented Aug 11, 2016

I already merged #7410 in the meantime but this might be a better solution.

@ghost ghost added the CLA Signed label Aug 14, 2016

@keyanzhang

This comment has been minimized.

Show comment
Hide comment
@keyanzhang

keyanzhang Aug 14, 2016

Member

Rebased and updated.
Here's a new benchmark that compares with the current master:

Comparing master.prod.txt (control) vs freeze.prod.txt (test)
Significant differences marked by ***
% change from control to test, with 99% CIs:

* factory_ms_jsc_jit
    % change:  -0.43% [ -5.25%,  +4.39%]
    means: 20.1321 (control), 20.0488 (test)
* factory_ms_jsc_nojit
    % change:  +0.72% [ -4.64%,  +6.08%]
    means: 18.2618 (control), 18.3966 (test)
* factory_ms_node
    % change:  +0.14% [ -2.57%,  +2.85%]
    means: 63.5866 (control), 63.6804 (test)
* ssr_pe_cold_ms_jsc_jit
    % change:  +1.98% [ -1.90%,  +5.87%]
    means: 29.2908 (control), 29.8738 (test)
* ssr_pe_cold_ms_jsc_nojit
    % change:  -1.68% [ -6.00%,  +2.64%]
    means: 31.9364 (control), 31.4052 (test)
* ssr_pe_cold_ms_node
    % change:  -0.21% [ -2.61%,  +2.20%]
    means: 58.2109 (control), 58.0934 (test)
Comparing master.dev.txt (control) vs freeze.dev.txt (test)
Significant differences marked by ***
% change from control to test, with 99% CIs:

* factory_ms_jsc_jit
    % change:  -2.74% [ -7.44%,  +1.97%]
    means: 27.3811 (control), 26.6391 (test)
* factory_ms_jsc_nojit
    % change:  +0.33% [ -4.34%,  +5.00%]
    means: 24.6727 (control), 24.7591 (test)
* factory_ms_node
    % change:  -0.37% [ -2.90%,  +2.17%]
    means: 90.5355 (control), 90.2092 (test)
* ssr_pe_cold_ms_jsc_jit
    % change:  -4.46% [ -7.64%,  -1.27%]  ***
    means: 87.2714 (control), 83.392 (test)
* ssr_pe_cold_ms_jsc_nojit
    % change:  -5.38% [ -9.78%,  -0.98%]  ***
    means: 93.9902 (control), 88.9515 (test)
* ssr_pe_cold_ms_node
    % change:  -4.82% [ -6.69%,  -2.95%]  ***
    means: 132.504 (control), 126.122 (test)
Member

keyanzhang commented Aug 14, 2016

Rebased and updated.
Here's a new benchmark that compares with the current master:

Comparing master.prod.txt (control) vs freeze.prod.txt (test)
Significant differences marked by ***
% change from control to test, with 99% CIs:

* factory_ms_jsc_jit
    % change:  -0.43% [ -5.25%,  +4.39%]
    means: 20.1321 (control), 20.0488 (test)
* factory_ms_jsc_nojit
    % change:  +0.72% [ -4.64%,  +6.08%]
    means: 18.2618 (control), 18.3966 (test)
* factory_ms_node
    % change:  +0.14% [ -2.57%,  +2.85%]
    means: 63.5866 (control), 63.6804 (test)
* ssr_pe_cold_ms_jsc_jit
    % change:  +1.98% [ -1.90%,  +5.87%]
    means: 29.2908 (control), 29.8738 (test)
* ssr_pe_cold_ms_jsc_nojit
    % change:  -1.68% [ -6.00%,  +2.64%]
    means: 31.9364 (control), 31.4052 (test)
* ssr_pe_cold_ms_node
    % change:  -0.21% [ -2.61%,  +2.20%]
    means: 58.2109 (control), 58.0934 (test)
Comparing master.dev.txt (control) vs freeze.dev.txt (test)
Significant differences marked by ***
% change from control to test, with 99% CIs:

* factory_ms_jsc_jit
    % change:  -2.74% [ -7.44%,  +1.97%]
    means: 27.3811 (control), 26.6391 (test)
* factory_ms_jsc_nojit
    % change:  +0.33% [ -4.34%,  +5.00%]
    means: 24.6727 (control), 24.7591 (test)
* factory_ms_node
    % change:  -0.37% [ -2.90%,  +2.17%]
    means: 90.5355 (control), 90.2092 (test)
* ssr_pe_cold_ms_jsc_jit
    % change:  -4.46% [ -7.64%,  -1.27%]  ***
    means: 87.2714 (control), 83.392 (test)
* ssr_pe_cold_ms_jsc_nojit
    % change:  -5.38% [ -9.78%,  -0.98%]  ***
    means: 93.9902 (control), 88.9515 (test)
* ssr_pe_cold_ms_node
    % change:  -4.82% [ -6.69%,  -2.95%]  ***
    means: 132.504 (control), 126.122 (test)
@keyanzhang

This comment has been minimized.

Show comment
Hide comment
@keyanzhang

keyanzhang Aug 30, 2016

Member

Friendly ping 😀: I just rebased and updated this PR. Do we want to take this approach to fix #7424?

To sum up, it calls .slice() when people explicitly pass an array as children (<Comp children={[...]}/> or <Comp>{[...]}<Comp/>) and freezes the sliced copy, so that we won't accidentally freeze userland data. We call slice in both prod and dev to align their behaviors, which regresses the prod build perf a little bit. A benchmark can be found above.

Member

keyanzhang commented Aug 30, 2016

Friendly ping 😀: I just rebased and updated this PR. Do we want to take this approach to fix #7424?

To sum up, it calls .slice() when people explicitly pass an array as children (<Comp children={[...]}/> or <Comp>{[...]}<Comp/>) and freezes the sliced copy, so that we won't accidentally freeze userland data. We call slice in both prod and dev to align their behaviors, which regresses the prod build perf a little bit. A benchmark can be found above.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 30, 2016

Member

Can you please help me understand how to read this profiler script output?
I don’t know if + is good or - is good.

Member

gaearon commented Aug 30, 2016

Can you please help me understand how to read this profiler script output?
I don’t know if + is good or - is good.

@keyanzhang

This comment has been minimized.

Show comment
Hide comment
@keyanzhang

keyanzhang Aug 30, 2016

Member

Yeah, sorry, - is good and + is bad. cc @spicyj

Member

keyanzhang commented Aug 30, 2016

Yeah, sorry, - is good and + is bad. cc @spicyj

@ghost ghost added the CLA Signed label Aug 30, 2016

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 30, 2016

Member

What does the benchmark measure?
Does it include memory usage?

Member

gaearon commented Aug 30, 2016

What does the benchmark measure?
Does it include memory usage?

@keyanzhang

This comment has been minimized.

Show comment
Hide comment
@keyanzhang

keyanzhang Aug 30, 2016

Member

No, it only measures time. The percentages (-2.74% [ -7.44%, +1.97%]) are mean, low, and high (https://github.com/facebook/react/blob/master/scripts/bench/analyze.py#L102-L107).

Member

keyanzhang commented Aug 30, 2016

No, it only measures time. The percentages (-2.74% [ -7.44%, +1.97%]) are mean, low, and high (https://github.com/facebook/react/blob/master/scripts/bench/analyze.py#L102-L107).

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 30, 2016

Member

I don’t think allocating more in production is the way to go here.
I think we should either keep this warning as is, or write a lint rule that warns on

this.props.children.<mutatingMethod>()

or

(var|const|let) <name> = (this.)?props.children;
<name>.{mutatingMethod}()

If this covers most cases and gets included into the default set of eslint-plugin-react, Create React App, and www codebase, we can remove the warning altogether IMO.

Member

gaearon commented Aug 30, 2016

I don’t think allocating more in production is the way to go here.
I think we should either keep this warning as is, or write a lint rule that warns on

this.props.children.<mutatingMethod>()

or

(var|const|let) <name> = (this.)?props.children;
<name>.{mutatingMethod}()

If this covers most cases and gets included into the default set of eslint-plugin-react, Create React App, and www codebase, we can remove the warning altogether IMO.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Aug 30, 2016

Member

It probably suffices to freeze the array if ReactElement creates it and do nothing otherwise, don't you think?

Member

sophiebits commented Aug 30, 2016

It probably suffices to freeze the array if ReactElement creates it and do nothing otherwise, don't you think?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 30, 2016

Member

I’m not sure when people would try to push things into children so hard to say.
Maybe @jimfb can show a few more examples of how people violated this warning in practice.
I thought that pushing things into an array created by map() is one of those use cases.

Member

gaearon commented Aug 30, 2016

I’m not sure when people would try to push things into children so hard to say.
Maybe @jimfb can show a few more examples of how people violated this warning in practice.
I thought that pushing things into an array created by map() is one of those use cases.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 30, 2016

Member

But I’m OK with interpretation above. We don’t have to catch every possible mistake, and what @spicyj suggested seems like good compromise to me.

Member

gaearon commented Aug 30, 2016

But I’m OK with interpretation above. We don’t have to catch every possible mistake, and what @spicyj suggested seems like good compromise to me.

@keyanzhang

This comment has been minimized.

Show comment
Hide comment
@keyanzhang

keyanzhang Aug 30, 2016

Member

Sounds good to me! I agree that we shouldn't introduce an overhead in prod.

Member

keyanzhang commented Aug 30, 2016

Sounds good to me! I agree that we shouldn't introduce an overhead in prod.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 12, 2016

Member

Could you rebase pls?

Member

gaearon commented Sep 12, 2016

Could you rebase pls?

@gaearon gaearon self-assigned this Sep 12, 2016

@ghost ghost added the CLA Signed label Sep 12, 2016

changed ReactChildrenMutationWarningHook to Object.freeze
- only freeze children array created by createElement
@keyanzhang

This comment has been minimized.

Show comment
Hide comment
@keyanzhang

keyanzhang Sep 12, 2016

Member

Sure, just rebased.

Member

keyanzhang commented Sep 12, 2016

Sure, just rebased.

@ghost ghost added the CLA Signed label Sep 12, 2016

@gaearon gaearon added this to the 15-next milestone Sep 13, 2016

@gaearon gaearon merged commit 38c4ade into facebook:master Sep 13, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.01%) to 87.295%
Details
@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 13, 2016

Member

Thanks!

Member

gaearon commented Sep 13, 2016

Thanks!

@zpao

This comment has been minimized.

Show comment
Hide comment
@zpao

zpao Sep 15, 2016

Member

FYI: There's no way this is a patch level change - it changes what was a warning to start throwing. Sure, it's dev-only but that's not ok to take in a patch release (we avoid introducing new warnings in patch releases, we definitely can't start throwing).

Let's be a bit more conservative with the semver assignment so we don't sneak breakages in at the wrong time.

Member

zpao commented Sep 15, 2016

FYI: There's no way this is a patch level change - it changes what was a warning to start throwing. Sure, it's dev-only but that's not ok to take in a patch release (we avoid introducing new warnings in patch releases, we definitely can't start throwing).

Let's be a bit more conservative with the semver assignment so we don't sneak breakages in at the wrong time.

@zpao zpao added semver-minor and removed semver-patch labels Sep 15, 2016

@just-boris

This comment has been minimized.

Show comment
Hide comment
@just-boris

just-boris Sep 16, 2016

@zpao does that mean that there will be 15.4 release soon?

just-boris commented Sep 16, 2016

@zpao does that mean that there will be 15.4 release soon?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 16, 2016

Member

I'll be more conservative, thanks for correcting!

Member

gaearon commented Sep 16, 2016

I'll be more conservative, thanks for correcting!

@zpao zpao modified the milestones: 15-next, 15.4.0 Oct 4, 2016

zpao added a commit that referenced this pull request Oct 4, 2016

changed ReactChildrenMutationWarningHook to Object.freeze (#7455)
- only freeze children array created by createElement
(cherry picked from commit 38c4ade)

@renovate renovate bot referenced this pull request Feb 2, 2018

Open

Update dependency react to v16 #29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment