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: use join vs run for deleteRecord destroy of new records #8307

Merged
merged 1 commit into from Nov 15, 2022

Conversation

runspired
Copy link
Contributor

resolves #8262

I'm not entirely convinced this is safe, though it passes tests. We're close to disentangling from the runloop entirely, should probably try to finish that off so we can be certain.

The rough cause of the issue is that Ember apparently now flushes render after every run, so nested runs are no longer safe. Unsure when this changed.

@runspired runspired added 🎯 release PR should be backported to release 🎯 canary PR is targeting canary (default) 🏷️ bug This PR primarily fixes a reported issue ci-perf Activates Performance Checks in CI labels Nov 15, 2022
@github-actions
Copy link

Performance Report for dcfb5de

Scenario - basic-record-materialization: ☑️ Performance is stable

☑️ duration
phase no difference [-20ms to 1ms]
☑️ Phase [navigationStart] => [start-data-generation]
phase no difference [-2ms to 1ms]
✅ Phase [start-data-generation] => [start-push-payload]
phase estimated improvement -3ms [-4ms to -1ms] OR -0.68% [-1.03% to -0.32%]
✅ Phase [start-push-payload] => [start-peek-records]
phase estimated improvement -8ms [-14ms to -1ms] OR -0.59% [-1.09% to -0.09%]
☑️ Phase [start-peek-records] => [start-record-materialization]
phase no difference [0ms to 0ms]
☑️ Phase [start-record-materialization] => [end-record-materialization]
phase no difference [-5ms to 6ms]
☑️ Phase [end-record-materialization] => [Test End]
phase no difference [0ms to 0ms]

Scenario - relationship-materialization-simple: ☑️ Performance is stable

☑️ duration
phase no difference [-5ms to 6ms]
☑️ Phase [navigationStart] => [start-find-all]
phase no difference [-1ms to 1ms]
☑️ Phase [start-find-all] => [start-materialization]
phase no difference [-5ms to 0ms]
☑️ Phase [start-materialization] => [end-materialization]
phase no difference [-8ms to 6ms]
☑️ Phase [end-materialization] => [Test End]
phase no difference [-2ms to 4ms]

Scenario - relationship-materialization-complex: ☑️ Performance is stable

☑️ duration
phase no difference [-21ms to 19ms]
☑️ Phase [navigationStart] => [start-data-generation]
phase no difference [-1ms to 2ms]
☑️ Phase [start-data-generation] => [start-push-payload]
phase no difference [0ms to 1ms]
✅ Phase [start-push-payload] => [start-peek-records]
phase estimated improvement -7ms [-11ms to -3ms] OR -1.04% [-1.72% to -0.4%]
☑️ Phase [start-peek-records] => [start-record-materialization]
phase no difference [0ms to 0ms]
☑️ Phase [start-record-materialization] => [start-relationship-materialization]
phase no difference [-3ms to 3ms]
☑️ Phase [start-relationship-materialization] => [end-relationship-materialization]
phase no difference [-11ms to 23ms]
☑️ Phase [end-relationship-materialization] => [Test End]
phase no difference [-4ms to 2ms]

Scenario - unload: ☑️ Performance is stable

☑️ duration
phase no difference [-9ms to 1ms]
☑️ Phase [navigationStart] => [start-push-payload]
phase no difference [-4ms to 0ms]
☑️ Phase [start-push-payload] => [start-unload-records]
phase no difference [-5ms to 1ms]
☑️ Phase [start-unload-records] => [end-unload-records]
phase no difference [-1ms to 3ms]
☑️ Phase [end-unload-records] => [Test End]
phase no difference [0ms to 0ms]

Scenario - unload-all: ✅ Performance improved

✅ duration
phase estimated improvement -10ms [-18ms to -2ms] OR -0.54% [-0.95% to -0.08%]
☑️ Phase [navigationStart] => [start-push-payload]
phase no difference [-1ms to 3ms]
✅ Phase [start-push-payload] => [start-materialization]
phase estimated improvement -11ms [-16ms to -6ms] OR -1.39% [-1.98% to -0.76%]
☑️ Phase [start-materialization] => [start-unload-all]
phase no difference [-3ms to 1ms]
☑️ Phase [start-unload-all] => [end-unload-all]
phase no difference [-3ms to 4ms]
☑️ Phase [end-unload-all] => [Test End]
phase no difference [0ms to 0ms]

Scenario - destroy: ☑️ Performance is stable

☑️ duration
phase no difference [-3ms to 6ms]
☑️ Phase [navigationStart] => [start-push-payload]
phase no difference [-1ms to 2ms]
☑️ Phase [start-push-payload] => [start-destroy-records]
phase no difference [-4ms to 2ms]
☑️ Phase [start-destroy-records] => [end-destroy-records]
phase no difference [-1ms to 3ms]
☑️ Phase [end-destroy-records] => [Test End]
phase no difference [0ms to 0ms]

Scenario - add-children: ☑️ Performance is stable

☑️ duration
phase no difference [-5ms to 5ms]
☑️ Phase [navigationStart] => [start-push-initial-payload]
phase no difference [-4ms to 3ms]
☑️ Phase [start-push-initial-payload] => [start-push-update-payload]
phase no difference [-1ms to 3ms]
☑️ Phase [start-push-update-payload] => [end-push-update-payload]
phase no difference [-3ms to 1ms]
☑️ Phase [end-push-update-payload] => [Test End]
phase no difference [0ms to 0ms]

Scenario - unused-relationships: ☑️ Performance is stable

☑️ duration
phase no difference [-3ms to 6ms]
☑️ Phase [navigationStart] => [start-push-payload]
phase no difference [-2ms to 2ms]
☑️ Phase [start-push-payload] => [end-push-payload]
phase no difference [-1ms to 5ms]
☑️ Phase [end-push-payload] => [Test End]
phase no difference [0ms to 0ms]

@runspired runspired merged commit da74d73 into master Nov 15, 2022
@delete-merged-branch delete-merged-branch bot deleted the fix-8262 branch November 15, 2022 01:34
@icole icole added 🎯 lts The PR should be backported to the most recent LTS and removed 🎯 release PR should be backported to release labels Dec 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-perf Activates Performance Checks in CI 🎯 canary PR is targeting canary (default) 🎯 lts The PR should be backported to the most recent LTS 🏷️ bug This PR primarily fixes a reported issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Ember Data 4.7] Error: Assertion Failed: Expected to receive a stable Identifier to subscribe to
2 participants