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: treat uncommitted mutations as local operations when receiving remote updates #9652

Merged
merged 2 commits into from
Feb 9, 2025

Conversation

gitKrystan
Copy link
Contributor

@gitKrystan gitKrystan commented Feb 6, 2025

We encountered a bug when upgrading to 4.13 (see also #9639) where the following series of steps resulted in an unexpected outcome in the deprecated code-path for handling of remoteState updates. We traced this down to an issue that originates when support for graph-diffing was first added in 5.x

Steps to reproduce:

  • await findRecord (backgroundReload)
  • updated hasMany to a new state
  • receive updated payload from backgroundReload with the exact same remote state as before
  • [expected] notice new hasMany state has been reverted
  • [actual (bug)] graph throws error

This can be more tersely described as

  • mutate a hasMany
  • receive remote state update for hasMany with exact remote state as before
  • [expected] notice hasMany mutation has been cleared
  • [actual (bug)] graph throws error

The error occurs because are treating the update to the mutation as an operation on remote state. It will be attempting to find the record added via mutation in the existing remote state of the inverse instead of the local state of the inverse.

@gitKrystan gitKrystan added 🎯 canary PR is targeting canary (default) 🏷️ bug This PR primarily fixes a reported issue labels Feb 6, 2025
@runspired runspired added the ci-perf Activates Performance Checks in CI label Feb 9, 2025
@runspired runspired changed the title What fails? fix: treat uncommitted mutations as local operations when receiving remote updates Feb 9, 2025
@runspired runspired self-assigned this Feb 9, 2025
Copy link

github-actions bot commented Feb 9, 2025

Performance Report for e418061

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

☑️ duration
phase no difference [-19ms to 25ms]
☑️ Phase [navigationStart] => [start-data-generation]
phase no difference [-1ms to 1ms]
☑️ Phase [start-data-generation] => [start-push-payload]
phase no difference [-2ms to 2ms]
☑️ Phase [start-push-payload] => [start-peek-records]
phase no difference [-9ms to 15ms]
☑️ Phase [start-peek-records] => [start-record-materialization]
phase no difference [0ms to 0ms]
☑️ Phase [start-record-materialization] => [end-record-materialization]
phase no difference [-8ms to 9ms]
☑️ Phase [end-record-materialization] => [Test End]
phase no difference [0ms to 1ms]

Scenario - relationship-materialization-simple: ✅ Performance improved

✅ duration
phase estimated improvement -7ms [-13ms to -2ms] OR -1.22% [-2.24% to -0.27%]
☑️ Phase [navigationStart] => [start-find-all]
phase no difference [0ms to 1ms]
✅ Phase [start-find-all] => [start-materialization]
phase estimated improvement -3ms [-5ms to 0ms] OR -1.29% [-2.37% to -0.14%]
✅ Phase [start-materialization] => [end-materialization]
phase estimated improvement -4ms [-7ms to -1ms] OR -1.62% [-2.79% to -0.4%]
☑️ Phase [end-materialization] => [Test End]
phase no difference [-1ms to 1ms]

Scenario - relationship-materialization-complex: ✅ Performance improved

✅ duration
phase estimated improvement -16ms [-32ms to -2ms] OR -0.6% [-1.2% to -0.08%]
☑️ Phase [navigationStart] => [start-data-generation]
phase no difference [0ms to 1ms]
☑️ Phase [start-data-generation] => [start-push-payload]
phase no difference [-1ms to 0ms]
✅ Phase [start-push-payload] => [start-peek-records]
phase estimated improvement -4ms [-7ms to -1ms] OR -0.9% [-1.63% to -0.15%]
☑️ Phase [start-peek-records] => [start-record-materialization]
phase no difference [0ms to 0ms]
☑️ Phase [start-record-materialization] => [start-relationship-materialization]
phase no difference [0ms to 5ms]
✅ Phase [start-relationship-materialization] => [end-relationship-materialization]
phase estimated improvement -18ms [-29ms to -7ms] OR -1.07% [-1.71% to -0.39%]
☑️ Phase [end-relationship-materialization] => [Test End]
phase no difference [0ms to 3ms]

Scenario - unload: ☑️ Performance is stable

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

Scenario - unload-all: ☑️ Performance is stable

☑️ duration
phase no difference [-4ms to 14ms]
☑️ Phase [navigationStart] => [start-push-payload]
phase no difference [-1ms to 1ms]
☑️ Phase [start-push-payload] => [start-materialization]
phase no difference [-2ms to 7ms]
☑️ Phase [start-materialization] => [start-unload-all]
phase no difference [-2ms to 5ms]
☑️ Phase [start-unload-all] => [end-unload-all]
phase no difference [-2ms to 5ms]
☑️ Phase [end-unload-all] => [Test End]
phase no difference [0ms to 0ms]

Scenario - destroy: ☑️ Performance is stable

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

Scenario - add-children: ☑️ Performance is stable

☑️ duration
phase no difference [-1ms to 5ms]
☑️ Phase [navigationStart] => [start-push-initial-payload]
phase no difference [-1ms to 1ms]
☑️ 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 [-1ms to 2ms]
☑️ Phase [end-push-update-payload] => [Test End]
phase no difference [-1ms to 1ms]

Scenario - unused-relationships: ☑️ Performance is stable

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

@runspired runspired removed the ci-perf Activates Performance Checks in CI label Feb 9, 2025
@runspired runspired merged commit ab86586 into main Feb 9, 2025
24 of 26 checks passed
@runspired runspired deleted the is-no-remote branch February 9, 2025 02:37
Copy link

github-actions bot commented Feb 9, 2025

Commit v Release Performance Report for e418061

Scenario - basic-record-materialization: ✅ Performance improved

✅ duration
phase estimated improvement -32ms [-44ms to -18ms] OR -1.91% [-2.6% to -1.07%]
☑️ Phase [navigationStart] => [start-data-generation]
phase no difference [0ms to 1ms]
☑️ Phase [start-data-generation] => [start-push-payload]
phase no difference [-1ms to 1ms]
✅ Phase [start-push-payload] => [start-peek-records]
phase estimated improvement -31ms [-38ms to -22ms] OR -3.29% [-4.06% to -2.4%]
☑️ Phase [start-peek-records] => [start-record-materialization]
phase no difference [0ms to 1ms]
☑️ Phase [start-record-materialization] => [end-record-materialization]
phase no difference [-3ms to 8ms]
✅ Phase [end-record-materialization] => [Test End]
phase estimated improvement -1ms [-1ms to 0ms] OR -2.6% [-5.43% to -0.97%]

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

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

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

☑️ duration
phase no difference [-21ms to 9ms]
☑️ Phase [navigationStart] => [start-data-generation]
phase no difference [-1ms to 0ms]
✅ Phase [start-data-generation] => [start-push-payload]
phase estimated improvement -1ms [-2ms to -1ms] OR -1.11% [-1.62% to -0.69%]
✅ Phase [start-push-payload] => [start-peek-records]
phase estimated improvement -15ms [-19ms to -12ms] OR -3.63% [-4.45% to -2.89%]
☑️ Phase [start-peek-records] => [start-record-materialization]
phase no difference [0ms to 0ms]
☑️ Phase [start-record-materialization] => [start-relationship-materialization]
phase no difference [-1ms to 4ms]
☑️ Phase [start-relationship-materialization] => [end-relationship-materialization]
phase no difference [-3ms to 19ms]
☑️ Phase [end-relationship-materialization] => [Test End]
phase no difference [0ms to 4ms]

Scenario - unload: ✅ Performance improved

✅ duration
phase estimated improvement -6ms [-9ms to -3ms] OR -1.1% [-1.63% to -0.59%]
✅ Phase [navigationStart] => [start-push-payload]
phase estimated improvement -1ms [-1ms to 0ms] OR -0.56% [-0.99% to -0.09%]
✅ Phase [start-push-payload] => [start-unload-records]
phase estimated improvement -5ms [-7ms to -3ms] OR -1.74% [-2.41% to -1.04%]
☑️ Phase [start-unload-records] => [end-unload-records]
phase no difference [-2ms to 0ms]
☑️ Phase [end-unload-records] => [Test End]
phase no difference [0ms to 0ms]

Scenario - unload-all: ✅ Performance improved

✅ duration
phase estimated improvement -28ms [-32ms to -24ms] OR -2.23% [-2.54% to -1.91%]
☑️ Phase [navigationStart] => [start-push-payload]
phase no difference [-2ms to 0ms]
✅ Phase [start-push-payload] => [start-materialization]
phase estimated improvement -18ms [-20ms to -15ms] OR -3.24% [-3.72% to -2.78%]
☑️ Phase [start-materialization] => [start-unload-all]
phase no difference [-3ms to 0ms]
✅ Phase [start-unload-all] => [end-unload-all]
phase estimated improvement -8ms [-10ms to -6ms] OR -3.17% [-3.95% to -2.38%]
☑️ Phase [end-unload-all] => [Test End]
phase no difference [0ms to 0ms]

Scenario - destroy: ✅ Performance improved

✅ duration
phase estimated improvement -4ms [-7ms to -1ms] OR -0.76% [-1.34% to -0.13%]
⚠️ Phase [navigationStart] => [start-push-payload]
phase estimated regression +1ms [0ms to 1ms] OR +0.54% [0.13% to 0.94%]
✅ Phase [start-push-payload] => [start-destroy-records]
phase estimated improvement -3ms [-6ms to -1ms] OR -1.27% [-2.05% to -0.49%]
☑️ Phase [start-destroy-records] => [end-destroy-records]
phase no difference [-4ms to 0ms]
☑️ Phase [end-destroy-records] => [Test End]
phase no difference [0ms to 0ms]

Scenario - add-children: ✅ Performance improved

✅ duration
phase estimated improvement -5ms [-7ms to -4ms] OR -1.34% [-1.8% to -0.88%]
☑️ Phase [navigationStart] => [start-push-initial-payload]
phase no difference [0ms to 1ms]
✅ Phase [start-push-initial-payload] => [start-push-update-payload]
phase estimated improvement -2ms [-4ms to 0ms] OR -1.49% [-3.05% to -0.15%]
✅ Phase [start-push-update-payload] => [end-push-update-payload]
phase estimated improvement -3ms [-5ms to -2ms] OR -3.56% [-4.89% to -2.22%]
☑️ Phase [end-push-update-payload] => [Test End]
phase no difference [0ms to 1ms]

Scenario - unused-relationships: ✅ Performance improved

✅ duration
phase estimated improvement -8ms [-10ms to -5ms] OR -1.78% [-2.34% to -1.22%]
☑️ Phase [navigationStart] => [start-push-payload]
phase no difference [-1ms to 0ms]
✅ Phase [start-push-payload] => [end-push-payload]
phase estimated improvement -7ms [-10ms to -5ms] OR -2.74% [-3.56% to -1.96%]
☑️ Phase [end-push-payload] => [Test End]
phase no difference [0ms to 0ms]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎯 canary PR is targeting canary (default) 🏷️ bug This PR primarily fixes a reported issue
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants