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.

Sign up for GitHub

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

Replace ConditionalWeakTable with simpler non-static Dictionary-based cache #2506

Merged
merged 16 commits into from
Sep 19, 2022
Merged

Replace ConditionalWeakTable with simpler non-static Dictionary-based cache #2506

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
7f2f539
Replace ConditionalWeakTable with MaterializerContext.AnnotationsCache
habbes Sep 9, 2022
fa9f651
Move up the scope of annotations cache to request-level instead of re…
habbes Sep 12, 2022
17ff489
Fix e2e tests
habbes Sep 12, 2022
b542679
Pass materializerContext argument to dynamically-called methods
habbes Sep 12, 2022
eabc99b
Make MaterializerAnnotatonsCache sealed
habbes Sep 15, 2022
b2bf1ee
Add doc comments to MaterializerAnnotationsCache
habbes Sep 15, 2022
53803d9
Remove ODataAnnotatableExtensions
habbes Sep 15, 2022
5a66220
Update doc comments
habbes Sep 15, 2022
819ca96
Update src/Microsoft.OData.Client/Materialization/EntityTrackingAdapt…
habbes Sep 17, 2022
e8c5792
Apply suggestions from code review
habbes Sep 17, 2022
13ced4d
Implement review suggestions
habbes Sep 17, 2022
f52c0a0
Remove whitespace
habbes Sep 17, 2022
a08b019
Use materializerContext parameter name instead of context
habbes Sep 17, 2022
3c9745f
Fix build errors
habbes Sep 17, 2022
b208a03
Rename MaterializerAnnotationsCache to MaterializerCache
habbes Sep 17, 2022
310e03b
Update src/Microsoft.OData.Client/Materialization/IODataMaterializerC…
habbes Sep 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions src/Microsoft.OData.Client/AtomMaterializerLog.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@ internal class AtomMaterializerLog
/// <summary>Target instance to refresh.</summary>
private object insertRefreshObject;

/// <summary>
/// The materializer context.
/// </summary>
private IODataMaterializerContext materializerContext;
habbes marked this conversation as resolved.
Show resolved Hide resolved

#endregion Private fields

#region Constructors
Expand All @@ -56,10 +61,11 @@ internal class AtomMaterializerLog
/// <param name="mergeOption">The merge option for the log.</param>
/// <param name="model">The model for the log.</param>
/// <param name="entityTracker">The entity tracker for the log.</param>
/// <param name="materializerContext">The materializer context.</param>
/// <remarks>
/// Note that the merge option can't be changed.
/// </remarks>
internal AtomMaterializerLog(MergeOption mergeOption, ClientEdmModel model, EntityTrackerBase entityTracker)
internal AtomMaterializerLog(MergeOption mergeOption, ClientEdmModel model, EntityTrackerBase entityTracker, IODataMaterializerContext materializerContext)
{
Debug.Assert(model != null, "model != null");
Debug.Assert(entityTracker != null, "entityTracker != null");
habbes marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -70,6 +76,7 @@ internal AtomMaterializerLog(MergeOption mergeOption, ClientEdmModel model, Enti
this.entityTracker = entityTracker;
this.identityStack = new Dictionary<Uri, ODataResource>(EqualityComparer<Uri>.Default);
this.links = new List<LinkDescriptor>();
this.materializerContext = materializerContext;
}

#endregion Constructors
Expand Down Expand Up @@ -179,7 +186,7 @@ internal void ApplyToContext()
foreach (KeyValuePair<Uri, ODataResource> entity in this.identityStack)
{
// Try to attach the entity descriptor got from materializer, if one already exists, get the existing reference instead.
MaterializerEntry entry = MaterializerEntry.GetEntry(entity.Value);
MaterializerEntry entry = MaterializerEntry.GetEntry(entity.Value, this.materializerContext);

bool mergeEntityDescriptorInfo = entry.CreatedByMaterializer ||
entry.ResolvedObject == this.insertRefreshObject ||
Expand Down Expand Up @@ -309,7 +316,7 @@ internal bool TryResolve(MaterializerEntry entry, out MaterializerEntry existing

if (this.identityStack.TryGetValue(entry.Id, out existingODataEntry))
{
existingEntry = MaterializerEntry.GetEntry(existingODataEntry);
existingEntry = MaterializerEntry.GetEntry(existingODataEntry, this.materializerContext);
return true;
}

Expand All @@ -321,7 +328,7 @@ internal bool TryResolve(MaterializerEntry entry, out MaterializerEntry existing
this.entityTracker.TryGetEntity(entry.Id, out state);
if (state == EntityStates.Unchanged)
{
existingEntry = MaterializerEntry.GetEntry(existingODataEntry);
existingEntry = MaterializerEntry.GetEntry(existingODataEntry, this.materializerContext);
return true;
}
else
Expand Down
6 changes: 6 additions & 0 deletions src/Microsoft.OData.Client/BaseAsyncResult.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ namespace Microsoft.OData.Client
using System.IO;
using System.Threading;
using Microsoft.OData;
using Microsoft.OData.Client.Materialization;

/// <summary>
/// Implementation of IAsyncResult
Expand All @@ -26,6 +27,11 @@ internal abstract class BaseAsyncResult : IAsyncResult
/// <summary>wrapped request</summary>
protected PerRequest perRequest;

/// <summary>
/// Cache used to store temporary metadata used for materialization of OData items.
/// </summary>
protected MaterializerCache materializerCache = new MaterializerCache();
habbes marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably want to expose this through the constructor rather than always using the default cache in case caches become configurable in the future

Copy link
Contributor Author

@habbes habbes Oct 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think it's a good idea, but since this is an internal class, I think it would be better to instantiate the cache until there's a need to make it configurable. This way, we think of it as an implementation detail of the BaseAsyncResult that the caller does not need to know about. If we pass it on the constructor, the caller will have to take responsibility for passing down the cache, and we might make the argument that it should be configurable at that level, all the way up to the DataServiceContext. But the buck has to stop somewhere since this isn't a user input. I think BaseAsyncResult is good place because it's what glues the request together, and this cache is scoped to a request.


/// <summary>
/// The int equivalent for true.
/// </summary>
Expand Down
6 changes: 4 additions & 2 deletions src/Microsoft.OData.Client/BatchSaveResult.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,8 @@ protected override MaterializeAtom GetMaterializer(EntityDescriptor entityDescri
queryComponents,
/*projectionPlan*/ null,
this.currentOperationResponse.CreateResponseMessage(),
ODataPayloadKind.Resource);
ODataPayloadKind.Resource,
this.materializerCache);
habbes marked this conversation as resolved.
Show resolved Hide resolved
}

/// <summary>
Expand Down Expand Up @@ -688,7 +689,8 @@ private IEnumerable<OperationResponse> HandleBatchResponse(ODataBatchReader batc
null,
this.currentOperationResponse.Headers.GetHeader(XmlConstants.HttpContentType),
this.currentOperationResponse.CreateResponseMessage(),
query.PayloadKind);
query.PayloadKind,
this.materializerCache);
Copy link
Contributor

Choose a reason for hiding this comment

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

base.materializerCache

qresponse = QueryOperationResponse.GetInstance(query.ElementType, this.currentOperationResponse.Headers, query, materializer);
}
}
Expand Down
7 changes: 5 additions & 2 deletions src/Microsoft.OData.Client/DataServiceRequest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ namespace Microsoft.OData.Client
using System.Linq;
using System.Net;
using Microsoft.OData;
using Microsoft.OData.Client.Materialization;

/// <summary>Non-generic placeholder for generic implementation</summary>
public abstract class DataServiceRequest
Expand Down Expand Up @@ -59,14 +60,16 @@ internal ODataPayloadKind PayloadKind
/// <param name="contentType">contentType</param>
/// <param name="message">the message</param>
/// <param name="expectedPayloadKind">expected payload kind.</param>
/// <param name="materializerCache">Cache used to store temporary metadata used for materialization of OData items.</param>
/// <returns>atom materializer</returns>
internal static MaterializeAtom Materialize(
ResponseInfo responseInfo,
QueryComponents queryComponents,
ProjectionPlan plan,
string contentType,
IODataResponseMessage message,
ODataPayloadKind expectedPayloadKind)
ODataPayloadKind expectedPayloadKind,
MaterializerCache materializerCache)
{
Debug.Assert(queryComponents != null, "querycomponents");
Debug.Assert(message != null, "message");
Copy link
Contributor

Choose a reason for hiding this comment

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

Debug.Assert(materializerCache != null, "materializerCache != null");

Expand All @@ -77,7 +80,7 @@ internal static MaterializeAtom Materialize(
return MaterializeAtom.EmptyResults;
}

return new MaterializeAtom(responseInfo, queryComponents, plan, message, expectedPayloadKind);
return new MaterializeAtom(responseInfo, queryComponents, plan, message, expectedPayloadKind, materializerCache);
}

/// <summary>
Expand Down
8 changes: 4 additions & 4 deletions src/Microsoft.OData.Client/Materialization/CollectionValueMaterializationPolicy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,11 @@ internal class CollectionValueMaterializationPolicy : MaterializationPolicy
/// <summary>
/// Initializes a new instance of the <see cref="CollectionValueMaterializationPolicy" /> class.
/// </summary>
/// <param name="context">The context.</param>
/// <param name="materializerContext">The context.</param>
/// <param name="primitivePolicy">The primitive policy.</param>
internal CollectionValueMaterializationPolicy(IODataMaterializerContext context, PrimitiveValueMaterializationPolicy primitivePolicy)
internal CollectionValueMaterializationPolicy(IODataMaterializerContext materializerContext, PrimitiveValueMaterializationPolicy primitivePolicy)
{
this.materializerContext = context;
this.materializerContext = materializerContext;
this.primitiveValueMaterializationPolicy = primitivePolicy;
}

Expand Down Expand Up @@ -134,7 +134,7 @@ internal void ApplyCollectionDataValues(
addValueToBackingICollectionInstance,
isElementNullable);

collectionProperty.SetMaterializedValue(collectionInstance);
collectionProperty.SetMaterializedValue(collectionInstance, this.materializerContext);
}

/// <summary>
Expand Down
6 changes: 4 additions & 2 deletions src/Microsoft.OData.Client/Materialization/EntityTrackingAdapter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,11 @@ internal class EntityTrackingAdapter
/// <param name="mergeOption">The merge option.</param>
/// <param name="model">The model.</param>
/// <param name="context">The context.</param>
internal EntityTrackingAdapter(EntityTrackerBase entityTracker, MergeOption mergeOption, ClientEdmModel model, DataServiceContext context)
/// <param name="materializerContext">The materialization context used to retrieve materialization related information.</param>

internal EntityTrackingAdapter(EntityTrackerBase entityTracker, MergeOption mergeOption, ClientEdmModel model, DataServiceContext context, IODataMaterializerContext materializerContext)
{
this.MaterializationLog = new AtomMaterializerLog(mergeOption, model, entityTracker);
this.MaterializationLog = new AtomMaterializerLog(mergeOption, model, entityTracker, materializerContext);
this.MergeOption = mergeOption;
this.EntityTracker = entityTracker;
this.Model = model;
Expand Down
20 changes: 10 additions & 10 deletions src/Microsoft.OData.Client/Materialization/EntryValueMaterializationPolicy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,16 @@ internal class EntryValueMaterializationPolicy : StructuralValueMaterializationP
/// <summary>
/// Initializes a new instance of the <see cref="EntryValueMaterializationPolicy" /> class.
/// </summary>
/// <param name="context">The context.</param>
/// <param name="materializerContext">The context.</param>
/// <param name="entityTrackingAdapter">The entity tracking adapter.</param>
/// <param name="lazyPrimitivePropertyConverter">The lazy primitive property converter.</param>
/// <param name="nextLinkTable">The next link table.</param>
internal EntryValueMaterializationPolicy(
IODataMaterializerContext context,
IODataMaterializerContext materializerContext,
EntityTrackingAdapter entityTrackingAdapter,
DSClient.SimpleLazy<PrimitivePropertyConverter> lazyPrimitivePropertyConverter,
Dictionary<IEnumerable, DataServiceQueryContinuation> nextLinkTable)
: base(context, lazyPrimitivePropertyConverter)
: base(materializerContext, lazyPrimitivePropertyConverter)
{
this.nextLinkTable = nextLinkTable;
this.EntityTrackingAdapter = entityTrackingAdapter;
Expand Down Expand Up @@ -534,11 +534,11 @@ private void ApplyFeedToCollection(
ClientEdmModel edmModel = this.MaterializerContext.Model;
ClientTypeAnnotation collectionType = edmModel.GetClientTypeAnnotation(edmModel.GetOrCreateEdmType(property.ResourceSetItemType));

IEnumerable<ODataResource> entries = MaterializerFeed.GetFeed(feed).Entries;
IEnumerable<ODataResource> entries = MaterializerFeed.GetFeed(feed, this.MaterializerContext).Entries;
habbes marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

base.MaterializerContext


foreach (ODataResource feedEntry in entries)
{
this.Materialize(MaterializerEntry.GetEntry(feedEntry), collectionType.ElementType, includeLinks);
this.Materialize(MaterializerEntry.GetEntry(feedEntry, this.MaterializerContext), collectionType.ElementType, includeLinks);
Copy link
Contributor

Choose a reason for hiding this comment

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

base.MaterializerContext

}

ProjectionPlan continuationPlan = includeLinks ?
Expand All @@ -548,7 +548,7 @@ private void ApplyFeedToCollection(
this.ApplyItemsToCollection(
entry,
property,
entries.Select(e => MaterializerEntry.GetEntry(e).ResolvedObject),
entries.Select(e => MaterializerEntry.GetEntry(e, this.MaterializerContext).ResolvedObject),
Copy link
Contributor

Choose a reason for hiding this comment

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

base.MaterializerContext

feed.NextPageLink,
continuationPlan,
false);
Expand Down Expand Up @@ -591,7 +591,7 @@ private void MaterializeResolvedEntry(MaterializerEntry entry, bool includeLinks

foreach (ODataNestedResourceInfo link in entry.NestedResourceInfos)
{
MaterializerNavigationLink linkState = MaterializerNavigationLink.GetLink(link);
MaterializerNavigationLink linkState = MaterializerNavigationLink.GetLink(link, this.MaterializerContext);
Copy link
Contributor

Choose a reason for hiding this comment

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

base.MaterializerContext


if (linkState == null)
{
Expand Down Expand Up @@ -722,7 +722,7 @@ private void MaterializeDynamicProperty(MaterializerEntry entry, ODataNestedReso
return;
}

MaterializerNavigationLink linkState = MaterializerNavigationLink.GetLink(link);
MaterializerNavigationLink linkState = MaterializerNavigationLink.GetLink(link, this.MaterializerContext);
Copy link
Contributor

Choose a reason for hiding this comment

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

base.MaterializerContext

if (linkState == null || (linkState.Entry == null && linkState.Feed == null))
{
return;
Expand Down Expand Up @@ -754,10 +754,10 @@ private void MaterializeDynamicProperty(MaterializerEntry entry, ODataNestedReso
Type collectionType = typeof(System.Collections.ObjectModel.Collection<>).MakeGenericType(new Type[] { collectionItemType });
IList collection = (IList)Util.ActivatorCreateInstance(collectionType);

IEnumerable<ODataResource> feedEntries = MaterializerFeed.GetFeed(linkState.Feed).Entries;
IEnumerable<ODataResource> feedEntries = MaterializerFeed.GetFeed(linkState.Feed, this.MaterializerContext).Entries;
Copy link
Contributor

Choose a reason for hiding this comment

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

base.MaterializerContext

foreach (ODataResource feedEntry in feedEntries)
{
MaterializerEntry linkEntry = MaterializerEntry.GetEntry(feedEntry);
MaterializerEntry linkEntry = MaterializerEntry.GetEntry(feedEntry, this.MaterializerContext);
Copy link
Contributor

Choose a reason for hiding this comment

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

base.MaterializerContext

this.Materialize(linkEntry, collectionItemType, false /*includeLinks*/);
collection.Add(linkEntry.ResolvedObject);
}
Expand Down
14 changes: 7 additions & 7 deletions src/Microsoft.OData.Client/Materialization/EnumValueMaterializationPolicy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,15 @@ namespace Microsoft.OData.Client.Materialization
internal class EnumValueMaterializationPolicy
{
/// <summary> MaterializerContext used to resolve types for materialization. </summary>
private readonly IODataMaterializerContext context;
private readonly IODataMaterializerContext materializerContext;

/// <summary>
/// Initializes a new instance of the <see cref="EnumValueMaterializationPolicy" /> class.
/// </summary>
/// <param name="context">The context.</param>
internal EnumValueMaterializationPolicy(IODataMaterializerContext context)
/// <param name="materializerContext">The materializer context.</param>
internal EnumValueMaterializationPolicy(IODataMaterializerContext materializerContext)
{
this.context = context;
this.materializerContext = materializerContext;
}

/// <summary>
Expand All @@ -40,9 +40,9 @@ public object MaterializeEnumTypeProperty(Type valueType, ODataProperty property
object materializedValue = null;
ODataEnumValue value = property.Value as ODataEnumValue;
this.MaterializeODataEnumValue(valueType, value.TypeName, value.Value, () => "TODO: Is this reachable?", out materializedValue);
if (!property.HasMaterializedValue())
if (!property.HasMaterializedValue(this.materializerContext))
{
property.SetMaterializedValue(materializedValue);
property.SetMaterializedValue(materializedValue, this.materializerContext);
}

return materializedValue;
Expand Down Expand Up @@ -102,7 +102,7 @@ private void MaterializeODataEnumValue(Type type, string wireTypeName, string en
{
Debug.Assert(type != null, "type != null");
Type underlyingType = Nullable.GetUnderlyingType(type) ?? type;
ClientTypeAnnotation elementTypeAnnotation = this.context.ResolveTypeForMaterialization(underlyingType, wireTypeName);
ClientTypeAnnotation elementTypeAnnotation = this.materializerContext.ResolveTypeForMaterialization(underlyingType, wireTypeName);
Debug.Assert(elementTypeAnnotation != null, "elementTypeAnnotation != null");

// TODO: Find better way to parse Enum
Expand Down
Loading