NightOwl888 commented on a change in pull request #410:
URL: https://github.com/apache/lucenenet/pull/410#discussion_r570114823
##########
File path: src/Lucene.Net.Facet/FacetsConfig.cs
##########
@@ -157,17 +153,14 @@ public virtual DimConfig GetDimConfig(string dimName)
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public virtual void SetHierarchical(string dimName, bool v)
{
- lock (syncLock)
+ // LUCENENET: Eliminated extra lookup by using TryGetValue instead
of ContainsKey
+ if (!fieldTypes.TryGetValue(dimName, out DimConfig fieldType))
Review comment:
After removing the lock, these operations are no longer atomic.
However you are correct that the `ConcurrentDictionary` can be used to
replace the lock by calling the appropriate overload of
[`AddOrUpdate`](https://docs.microsoft.com/en-us/dotnet/api/system.collections.concurrent.concurrentdictionary-2.addorupdate?view=net-5.0).
Please change to use this `AddOrUpdate` rather than multiple method calls on
the `ConcurrentDictionary`.
##########
File path: src/Lucene.Net.Facet/FacetsConfig.cs
##########
@@ -178,17 +171,14 @@ public virtual void SetHierarchical(string dimName, bool
v)
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public virtual void SetMultiValued(string dimName, bool v)
{
- lock (syncLock)
+ // LUCENENET: Eliminated extra lookup by using TryGetValue instead
of ContainsKey
+ if (!fieldTypes.TryGetValue(dimName, out DimConfig fieldType))
Review comment:
After removing the lock, these operations are no longer atomic.
However you are correct that the `ConcurrentDictionary` can be used to
replace the lock by calling the appropriate overload of
[`AddOrUpdate`](https://docs.microsoft.com/en-us/dotnet/api/system.collections.concurrent.concurrentdictionary-2.addorupdate?view=net-5.0).
Please change to use this `AddOrUpdate` rather than multiple method calls on
the `ConcurrentDictionary`.
##########
File path: src/Lucene.Net.Facet/FacetsConfig.cs
##########
@@ -221,17 +208,14 @@ public virtual void SetRequireDimCount(string dimName,
bool v)
/// </summary>
public virtual void SetIndexFieldName(string dimName, string
indexFieldName)
{
- lock (syncLock)
+ // LUCENENET: Eliminated extra lookup by using TryGetValue instead
of ContainsKey
+ if (!fieldTypes.TryGetValue(dimName, out DimConfig fieldType))
Review comment:
After removing the lock, these operations are no longer atomic.
However you are correct that the `ConcurrentDictionary` can be used to
replace the lock by calling the appropriate overload of
[`AddOrUpdate`](https://docs.microsoft.com/en-us/dotnet/api/system.collections.concurrent.concurrentdictionary-2.addorupdate?view=net-5.0).
Please change to use this `AddOrUpdate` rather than multiple method calls on
the `ConcurrentDictionary`.
##########
File path: src/Lucene.Net.Facet/FacetsConfig.cs
##########
@@ -200,17 +190,14 @@ public virtual void SetMultiValued(string dimName, bool v)
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public virtual void SetRequireDimCount(string dimName, bool v)
{
- lock (syncLock)
+ // LUCENENET: Eliminated extra lookup by using TryGetValue instead
of ContainsKey
+ if (!fieldTypes.TryGetValue(dimName, out DimConfig fieldType))
Review comment:
After removing the lock, these operations are no longer atomic.
However you are correct that the `ConcurrentDictionary` can be used to
replace the lock by calling the appropriate overload of
[`AddOrUpdate`](https://docs.microsoft.com/en-us/dotnet/api/system.collections.concurrent.concurrentdictionary-2.addorupdate?view=net-5.0).
Please change to use this `AddOrUpdate` rather than multiple method calls on
the `ConcurrentDictionary`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
