Skip to content

Replaced IdentityMap with IdentityMap<TK, TV> - #3787

Open
Fube wants to merge 1 commit into
nhibernate:masterfrom
Fube:fube/opt_snapshotdict
Open

Replaced IdentityMap with IdentityMap<TK, TV>#3787
Fube wants to merge 1 commit into
nhibernate:masterfrom
Fube:fube/opt_snapshotdict

Conversation

@Fube

@Fube Fube commented Aug 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Continues from #3762 (comment)
Addresses #3788

Benchmarks before

Method EntryCount Mean Error StdDev Median Gen0 Gen1 Gen2 Allocated
Flush_NoPendingChanges 1000 164.94 ms 3.278 ms 6.315 ms 166.17 ms 21000.0000 1000.0000 - 253.01 MB
Flush_WithDirtyEntities 1000 494.30 ms 9.154 ms 19.107 ms 486.58 ms 40000.0000 2000.0000 1000.0000 485.4 MB
Flush_WithNewWrites 1000 42.43 ms 0.842 ms 1.285 ms 42.32 ms 2000.0000 1000.0000 - 24.78 MB
Flush_NoPendingChanges 5000 996.19 ms 18.029 ms 20.039 ms 991.64 ms 109000.0000 31000.0000 11000.0000 1264.68 MB
Flush_WithDirtyEntities 5000 3,406.53 ms 67.236 ms 77.429 ms 3,394.60 ms 207000.0000 72000.0000 12000.0000 2426.96 MB
Flush_WithNewWrites 5000 217.27 ms 3.666 ms 3.061 ms 217.55 ms 12000.0000 4000.0000 2000.0000 123.31 MB

Benchmarks after

Method EntryCount Mean Error StdDev Gen0 Gen1 Gen2 Allocated
Flush_NoPendingChanges 1000 143.63 ms 2.867 ms 4.112 ms 18000.0000 1000.0000 - 222.13 MB
Flush_WithDirtyEntities 1000 457.79 ms 8.116 ms 9.967 ms 37000.0000 3000.0000 1000.0000 454.52 MB
Flush_WithNewWrites 1000 38.44 ms 0.763 ms 1.690 ms 2000.0000 1000.0000 - 24.31 MB
Flush_NoPendingChanges 5000 764.26 ms 14.852 ms 19.312 ms 93000.0000 2000.0000 1000.0000 1110.19 MB
Flush_WithDirtyEntities 5000 2,966.58 ms 38.978 ms 34.553 ms 200000.0000 73000.0000 11000.0000 2272.48 MB
Flush_WithNewWrites 5000 197.08 ms 3.895 ms 7.689 ms 11000.0000 4000.0000 2000.0000 122.43 MB

@Fube
Fube force-pushed the fube/opt_snapshotdict branch from 0767ea9 to e751ae1 Compare August 12, 2026 13:24
@Fube
Fube marked this pull request as ready for review August 12, 2026 20:37
@hazzik

hazzik commented Aug 13, 2026

Copy link
Copy Markdown
Member

@Fube need to regenerate API change files

@Fube

Fube commented Aug 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@Fube need to regenerate API change files

@hazzik I ran dotnet format analyzers ./src/NHibernate/NHibernate.csproj --diagnostics RS0016 --include-generated per CONTRIBUTING.MD and it yielded no changes.
Is there something else I am supposed to run / update?

@hazzik

hazzik commented Aug 13, 2026

Copy link
Copy Markdown
Member

Yes, update branch first

@Fube
Fube force-pushed the fube/opt_snapshotdict branch from e751ae1 to f044fe3 Compare August 13, 2026 12:16
@Fube

Fube commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

Yes, update branch first

Rebased and reran the command.
Still no difference.
I am not sure anything should change as my PR does not introduce anything new to the library's public API.

hazzik
hazzik reviewed Aug 16, 2026
View reviewed changes
Comment thread src/NHibernate/Util/IdentityMap.cs
@Fube
Fube force-pushed the fube/opt_snapshotdict branch from f044fe3 to 99cf8dd Compare August 18, 2026 13:12
@Fube
Fube force-pushed the fube/opt_snapshotdict branch from 99cf8dd to 885eda4 Compare August 18, 2026 13:14
@Fube

Fube commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

@hazzik Do you need anything else from me?

@hazzik

hazzik commented Sep 1, 2026

Copy link
Copy Markdown
Member

Do you need anything else

Time, I need time.

hazzik
hazzik reviewed Sep 2, 2026
View reviewed changes
Comment thread src/NHibernate/Engine/Loading/LoadContexts.cs
if (collectionLoadContexts != null)
{
CollectionLoadContext collectionLoadContext = (CollectionLoadContext)collectionLoadContexts[resultSet];
collectionLoadContexts.TryGetValue(resultSet, out var collectionLoadContext);

@hazzik hazzik Sep 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think here we need semantics of TryRemove

hazzik
hazzik reviewed Sep 2, 2026
View reviewed changes
Comment thread src/NHibernate/Engine/StatefulPersistenceContext.cs
((IPersistentCollection)entry.Key).UnsetSession(Session);
foreach (var entry in collectionEntryArray)
{
((IPersistentCollection)entry.Key).UnsetSession(Session);

@hazzik hazzik Sep 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we make key of the dictionary a IPersistentCollection?

hazzik
hazzik reviewed Sep 2, 2026
View reviewed changes
Comment thread src/NHibernate/Engine/StatefulPersistenceContext.cs
public EntityEntry GetEntry(object entity)
{
return (EntityEntry)entityEntries[entity];
if (entityEntries.TryGetValue(entity, out var entry))

@hazzik hazzik Sep 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same value typed as EntityEntry?

@hazzik

hazzik commented Sep 2, 2026

Copy link
Copy Markdown
Member

Can you please reformat:

dotnet restore src/NHibernate.sln 
dotnet format whitespace src/NHibernate.sln \
  --include $(git diff --name-only upstream/master...HEAD -- '*.cs')

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

Reviewers

@hazzik hazzik hazzik left review comments
+1 more reviewer
@amilia-nabrari amilia-nabrari amilia-nabrari left review comments
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

t: Fix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Fube @hazzik @amilia-nabrari @fredericDelaporte