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

Fix for tracked properties updating when using m3 native properties #1403

Merged
merged 5 commits into from
Nov 1, 2021

Conversation

igorT
Copy link
Collaborator

@igorT igorT commented Oct 12, 2021

The fix in #1376 was not correct. In order to properly entangle we need to be doing receiver.get but to protect against re-entrance in production, we need to manually keep track of the key being accessed.

@github-actions
Copy link

github-actions bot commented Oct 12, 2021

Performance Report for b010003

Scenario - materializing-lots: ⚠️ Performance regressed

⚠️ duration
phase estimated regression +101ms [53ms to 147ms] OR +1.06% [0.55% to 1.53%]
☑️ Phase [navigationStart] => [start-loading]
phase no difference [-3ms to 8ms]
⚠️ Phase [start-loading] => [pushed-payload]
phase estimated regression +63ms [37ms to 90ms] OR +1.4% [0.81% to 2%]
⚠️ Phase [pushed-payload] => [end-loading]
phase estimated regression +33ms [4ms to 60ms] OR +0.72% [0.08% to 1.32%]
☑️ Phase [end-loading] => [Test End]
phase no difference [-1ms to 1ms]

Scenario - materializing-small: ☑️ Performance is stable

☑️ duration
phase no difference [-6ms to 6ms]
☑️ Phase [navigationStart] => [start-loading]
phase no difference [-6ms to 4ms]
☑️ Phase [start-loading] => [pushed-payload]
phase no difference [-1ms to 2ms]
☑️ Phase [pushed-payload] => [end-loading]
phase no difference [-1ms to 2ms]
☑️ Phase [end-loading] => [Test End]
phase no difference [-1ms to 1ms]

Scenario - rendering: ☑️ Performance is stable

☑️ duration
phase no difference [-10ms to 9ms]
☑️ Phase [navigationStart] => [start-loading]
phase no difference [-2ms to 5ms]
⚠️ Phase [start-loading] => [pushed-payload]
phase estimated regression +2ms [0ms to 3ms] OR +2.07% [0.49% to 3.81%]
☑️ Phase [pushed-payload] => [end-loading]
phase no difference [-1ms to 0ms]
☑️ Phase [end-loading] => [Test End]
phase no difference [-9ms to 5ms]

@igorT igorT force-pushed the igor/fix-template-updating branch 2 times, most recently from e9e39c0 to 2e40700 Compare October 28, 2021 00:47
@igorT igorT marked this pull request as ready for review October 28, 2021 00:47
@igorT igorT changed the title Fix for template tracked props updating Fix for tracked properties updating when using m3 native properties Oct 28, 2021
@igorT igorT requested review from hjdivad and rwjblue and removed request for hjdivad October 28, 2021 00:49
Copy link
Collaborator

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Given the explanation in the comments, I think there is probably a more straight forward solution for us.

Do we think (with the new tests added) that all of the cases that we care about are handled?

addon/model.js Outdated Show resolved Hide resolved
Co-authored-by: Robert Jackson <rjackson@linkedin.com>
@igorT
Copy link
Collaborator Author

igorT commented Nov 1, 2021

Do we think (with the new tests added) that all of the cases that we care about are handled
I am not aware of any, do any come to mind?

addon/model.js Outdated Show resolved Hide resolved
Co-authored-by: Robert Jackson <rjackson@linkedin.com>
Copy link
Collaborator

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Chatted a bit with @igorT about this (and my concerns in the prior comment). I think this is fine for now (with the tweak to use try/finally instead of relying on the nested access to reset), but we should work to refactor away from all of this.

The path forward is basically to migrate to using @glimmer/tracking/primitives/storage (which allows us to remove the need for Ember.get in the proxy handler's get hook, by ensuring that our own tags are property consumed/entangled) via the https://github.com/ember-polyfills/ember-tracked-storage-polyfill. Once we do that then we can make our proxy return undefined for both unknownProperty and setUknownProperty.

Those changes should likely be follow on work after this lands.

@rwjblue rwjblue added the bug ch:bugfix label Nov 1, 2021
@igorT igorT merged commit 81fd2d5 into master Nov 1, 2021
@igorT igorT deleted the igor/fix-template-updating branch November 1, 2021 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug ch:bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants