Skip to content

Conversation

DamirLisak
Copy link

@DamirLisak DamirLisak commented Aug 12, 2025

  • [x ] I've read the guidelines for contributing and seen the walkthrough
  • [x ] I've posted a comment on an issue with a detailed description of how I am planning to contribute and got approval from a member of the team
  • [x ] The code builds and tests pass locally (also verified by our automated build checks)
  • [x ] Commit messages follow this format:
Summary of the changes
  • Tests for the changes have been added (for bug fixes / features)
  • Code follows the same patterns and style as existing code in this repo

Issue #31819 "DetectChanges slow in long-term contexts" is no longer necessary if all entity classes in the model are defined with a change-tracking strategy other than "ChangeTrackingStrategy.Snapshot." This is controlled in the ProcessModelFinalizing method of the ChangeTrackingStrategyConvention class. DetectChanges is therefore not called when saving because change tracking is performed using the EntityReferenceMap (_entityReferenceMap property), which essentially corresponds to the previous caching mechanism using EntityState dictionaries from EF4/5/6. However, querying this cache was not enabled; instead, users were forced to use ChangeTracker.Entries(), which returned all objects and led to performance problems with large long-term contexts. This change now enables fast access to changed objects using the new GetEntriesForState method in the ChangeTracker class. This, in turn, calls EntityReferenceMap.GetEntriesForState(), which already exists.

Issue #16491: Implemented legacy Merge-Option Feature from EF/4/5/6

DamirLisak and others added 4 commits August 7, 2025 10:09
…onger necessary if all entity classes in the model are defined with a change-tracking strategy other than "ChangeTrackingStrategy.Snapshot." This is controlled in the ProcessModelFinalizing method of the ChangeTrackingStrategyConvention class. DetectChanges is therefore not called when saving because change tracking is performed using the EntityReferenceMap (_entityReferenceMap property), which essentially corresponds to the previous caching mechanism using EntityState dictionaries from EF4/5/6. However, querying this cache was not enabled; instead, users were forced to use ChangeTracker.Entries(), which returned all objects and led to performance problems with large long-term contexts. This change now enables fast access to changed objects using the new GetEntriesForState method in the ChangeTracker class. This, in turn, calls EntityReferenceMap.GetEntriesForState(), which already exists.

For fast change tracking, implement INotifyProperty-Changed on your entity classes and activate this strategy in the Model Builder using ChangeTrackingStrategy.ChangedNotifications via HasChangeTrackingStrategy().
dotnet#31819
@cincuranet
Copy link
Contributor

This needs to target main. If it meets the bar for 9.0 servicing, we will backport it.

@DamirLisak
Copy link
Author

@dotnet-policy-service agree

@roji
Copy link
Member

roji commented Aug 12, 2025

As this seems to be a new feature, there's very little chance we'll be backporting this to 9.0. In fact, the window for introducing new features into 10.0 is also closing very soon, so this will most likely be considered for 11.0 at the earliest.

@AndriySvyryd AndriySvyryd changed the title Release/9.0 Implement MergeOption Aug 13, 2025
@AndriySvyryd AndriySvyryd self-assigned this Aug 13, 2025
@DamirLisak
Copy link
Author

As this seems to be a new feature, there's very little chance we'll be backporting this to 9.0. In fact, the window for introducing new features into 10.0 is also closing very soon, so this will most likely be considered for 11.0 at the earliest.

Do I need to do anything else on my side, or can you handle the remaining work yourself so it can be included in release 10 or 11? (I'm asking because I'm not familiar with GitHub workflows or merging into the main branch.)

@roji
Copy link
Member

roji commented Aug 13, 2025

As @cincuranet wrote above, you need retarget this PR against main - it's currently targeting release/9.0. And as I wrote above, this will almost certainly not going into EF 10 (or 9).

@DamirLisak
Copy link
Author

@cincuranet How can I "retarget to main"? Do you have any instructions?

@cincuranet
Copy link
Contributor

Rebase your changes on top of main. Then Edit this PR and change the target branch (or close this one and open new one).

…onger necessary if all entity classes in the model are defined with a change-tracking strategy other than "ChangeTrackingStrategy.Snapshot." This is controlled in the ProcessModelFinalizing method of the ChangeTrackingStrategyConvention class. DetectChanges is therefore not called when saving because change tracking is performed using the EntityReferenceMap (_entityReferenceMap property), which essentially corresponds to the previous caching mechanism using EntityState dictionaries from EF4/5/6. However, querying this cache was not enabled; instead, users were forced to use ChangeTracker.Entries(), which returned all objects and led to performance problems with large long-term contexts. This change now enables fast access to changed objects using the new GetEntriesForState method in the ChangeTracker class. This, in turn, calls EntityReferenceMap.GetEntriesForState(), which already exists.

For fast change tracking, implement INotifyProperty-Changed on your entity classes and activate this strategy in the Model Builder using ChangeTrackingStrategy.ChangedNotifications via HasChangeTrackingStrategy().
dotnet#31819
@DamirLisak DamirLisak changed the base branch from release/9.0 to main August 15, 2025 09:14
@DamirLisak DamirLisak requested a review from a team as a code owner August 15, 2025 09:14
@cincuranet cincuranet removed the request for review from dotnet-maestro-bot August 15, 2025 10:36
Copy link
Member

@AndriySvyryd AndriySvyryd left a comment

Choose a reason for hiding this comment

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

This needs a significant number of new tests to be added.

  • Some tests for GetEntriesForState to exercise each parameter
  • A new test class for Refresh - test\EFCore.Specification.Tests\MergeOptionTestBase.cs
    • It should cover the following cases:
      • Existing entries in all states
      • Unchanged entries with original value set to something that doesn't match the database state
      • Modified entries with properties marked as modified, but with the original value set to something that matches the database state
      • Owned entity that was replaced by a different instance, so it's tracked as both Added and Deleted
      • A derived entity that was replaced by a base entity with same key value
      • Different terminating operators: ToList, FirstOrDefault, etc...
      • Streaming (non-buffering) query that's consumed one-by-one
      • Queries with Include, Include with filter and ThenInclude
      • Projecting a related entity in Select without Include
      • Creating a new instance of the target entity in Select with calculated values that are going to be client-evaluated
      • Projecting an entity multiple times in Select with same key, but different property values
      • Lazy-loading proxies with navigations in loaded and unloaded states
      • Non-tracking queries should throw
      • Multiple Refresh with different values in the same query should throw
    • The test model should incorporate the following features:
      • Collection and non-collection owned types
      • Collection and non-collection complex properties of value and reference types
      • Many-to-many relationships without an explicit join type
      • Global query filters
      • Shadow and non-shadow properties
      • Properties mapped to computed columns
      • Properties with value converters
      • Primitive collections
      • Table-sharing with shared non-key columns

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

Successfully merging this pull request may close these issues.

4 participants