NightOwl888 commented on code in PR #938:
URL: https://github.com/apache/lucenenet/pull/938#discussion_r1597522965
##########
src/Lucene.Net.Tests/Support/TestConcurrentHashSet.cs:
##########
@@ -0,0 +1,51 @@
+using Lucene.Net.Attributes;
+using Lucene.Net.Support;
+using NUnit.Framework;
+using System.Linq;
+using System.Threading.Tasks;
+
+namespace Lucene.Net
+{
+ /*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+ public class TestConcurrentHashSet
Review Comment:
Unfortunately, the author of this collection didn't provide any tests.
https://github.com/i3arnon/ConcurrentHashSet. But since it is based on
`ConcurrentDictionary`, maybe we could also base the tests on the
`ConcurrentDictionary` tests...?
As you have noticed, it is also not fully implemented, otherwise it could be
moved to J2N.Colllections.Concurrent with the rest of the collections.
These issues may be relevant:
- https://github.com/dotnet/runtime/issues/16443
- https://github.com/dotnet/runtime/issues/39919
I disagree with them about the benefits of this, though.
`ConcurrentDictionary` doens't implment `ISet<T>` and therefore cannot be
plugged into an API that accepts the interface. It also makes set operations
such as `IntersectWith` hard to deal with.
##########
src/Lucene.Net/Util/IOUtils.cs:
##########
@@ -516,8 +519,53 @@ public static void ReThrowUnchecked(Exception th)
}
}
- // LUCENENET specific: Fsync is pointless in .NET, since we are
- // calling FileStream.Flush(true) before the stream is disposed
- // which means we never need it at the point in Java where it is
called.
+ public static void Fsync(FileSystemInfo fileToSync, bool isDir)
Review Comment:
With `FileSystemInfo`, there is no reason for the boolean parameter. We can
check whether a `DirectoryInfo` or `FileInfo` object is passed by using
`fileToSync is DirectoryInfo` or `fileToSync is FileInfo`.
That said, it would make more sense to pass a string and boolean here as
they did in Java because we can remove the allocations of creating these
objects in .NET. All of the .NET APIs accept a string instead of a
`FileSystemInfo`. See #832.
##########
src/Lucene.Net/Support/IO/WindowsFsyncSupport.cs:
##########
@@ -0,0 +1,119 @@
+using System;
+using System.IO;
+using System.Runtime.InteropServices;
+
+namespace Lucene.Net.Support.IO
+{
+ /*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+ public static class WindowsFsyncSupport
Review Comment:
I noticed that other projects follow a consistent naming convention ending
in a `.<platform>.cs` suffix. I have seen some projects even make a distinction
between `Windows` and `Win32`. I believe this would go under `Win32`, though.
I think we should consider how adding more native calls would work and
follow Microsoft's lead on this. Maybe we should put all native code in a
`Support/Native` folder and use nested classes like `Interop.Win32.FSync`.
Thoughts?
##########
src/Lucene.Net/Support/ConcurrentHashSet.cs:
##########
@@ -753,7 +753,16 @@ private void CopyToItems(T[] array, int index)
public void ExceptWith(IEnumerable<T> other)
{
- throw new NotImplementedException();
+ if (ReferenceEquals(this, other))
Review Comment:
This is not a threadsafe implementation - this operation should be atomic.
Please see the implementation of `UnionWith` to lock the set for updating. Do
note, however, that implementation is untested like the rest of the collection
(other than not known to fail in integration tests).
##########
src/Lucene.Net/Util/IOUtils.cs:
##########
@@ -516,8 +519,53 @@ public static void ReThrowUnchecked(Exception th)
}
}
- // LUCENENET specific: Fsync is pointless in .NET, since we are
- // calling FileStream.Flush(true) before the stream is disposed
- // which means we never need it at the point in Java where it is
called.
+ public static void Fsync(FileSystemInfo fileToSync, bool isDir)
+ {
+ // LUCENENET NOTE: there is a bug in 4.8 where it tries to fsync a
directory on Windows,
+ // which is not supported in OpenJDK. This change adopts the
latest Lucene code as of 9.10
+ // and only fsyncs directories on Linux and macOS.
+
+ // If the file is a directory we have to open read-only, for
regular files we must open r/w for
+ // the fsync to have an effect.
+ // See
http://blog.httrack.com/blog/2013/11/15/everything-you-always-wanted-to-know-about-fsync/
+ if (isDir && Constants.WINDOWS)
+ {
+ // opening a directory on Windows fails, directories can not
be fsynced there
+ if (System.IO.Directory.Exists(fileToSync.FullName) == false)
+ {
+ // yet do not suppress trying to fsync directories that do
not exist
+ throw new DirectoryNotFoundException($"Directory does not
exist: {fileToSync}");
+ }
+ return;
+ }
+
+ try
+ {
+ // LUCENENET specific: was: file.force(true);
+ // We must call fsync on the parent directory, requiring some
custom P/Invoking
+ if (Constants.WINDOWS)
+ {
+ WindowsFsyncSupport.Fsync(fileToSync.FullName, isDir);
+ }
+ else
+ {
+ PosixFsyncSupport.Fsync(fileToSync.FullName, isDir);
+ }
+ }
+ catch (Exception e) when (e.IsIOException())
+ {
+ if (isDir)
+ {
+ Debug.Assert((Constants.LINUX || Constants.MAC_OS_X) ==
false,
Review Comment:
Please use our `Debugging.Assert` method here. For performance reasons, it
must go inside of a `if (Debugging.AssertsEnabled)` block to be as cheap as
possible in production while still retaining the ability to assert during
testing.
See https://github.com/apache/lucenenet/issues/346#issuecomment-696780561.
Please note that we don't want this call to do any string formatting or
concatenation inline, as that directly impacts the runtime performance of the
code. So, in this case, we would want to pass the `e` as a parameter to
`Debugging.Assert` and use a format string to insert it. The Assert method
doesn't do any string manipulation unless the assert fails. And although it
affects readability, we should eliminate the string concatenation here.
##########
src/Lucene.Net/Util/IOUtils.cs:
##########
@@ -516,8 +519,53 @@ public static void ReThrowUnchecked(Exception th)
}
}
- // LUCENENET specific: Fsync is pointless in .NET, since we are
- // calling FileStream.Flush(true) before the stream is disposed
- // which means we never need it at the point in Java where it is
called.
+ public static void Fsync(FileSystemInfo fileToSync, bool isDir)
+ {
+ // LUCENENET NOTE: there is a bug in 4.8 where it tries to fsync a
directory on Windows,
+ // which is not supported in OpenJDK. This change adopts the
latest Lucene code as of 9.10
+ // and only fsyncs directories on Linux and macOS.
+
+ // If the file is a directory we have to open read-only, for
regular files we must open r/w for
+ // the fsync to have an effect.
+ // See
http://blog.httrack.com/blog/2013/11/15/everything-you-always-wanted-to-know-about-fsync/
+ if (isDir && Constants.WINDOWS)
+ {
+ // opening a directory on Windows fails, directories can not
be fsynced there
+ if (System.IO.Directory.Exists(fileToSync.FullName) == false)
+ {
+ // yet do not suppress trying to fsync directories that do
not exist
+ throw new DirectoryNotFoundException($"Directory does not
exist: {fileToSync}");
+ }
+ return;
+ }
+
+ try
+ {
+ // LUCENENET specific: was: file.force(true);
+ // We must call fsync on the parent directory, requiring some
custom P/Invoking
+ if (Constants.WINDOWS)
+ {
+ WindowsFsyncSupport.Fsync(fileToSync.FullName, isDir);
+ }
+ else
+ {
+ PosixFsyncSupport.Fsync(fileToSync.FullName, isDir);
+ }
+ }
+ catch (Exception e) when (e.IsIOException())
+ {
+ if (isDir)
+ {
+ Debug.Assert((Constants.LINUX || Constants.MAC_OS_X) ==
false,
Review Comment:
Please use our `Debugging.Assert` method here. For performance reasons, it
must go inside of a `if (Debugging.AssertsEnabled)` block to be as cheap as
possible in production.
See https://github.com/apache/lucenenet/issues/346#issuecomment-696780561.
##########
src/Lucene.Net/Support/ConcurrentHashSet.cs:
##########
@@ -753,7 +753,16 @@ private void CopyToItems(T[] array, int index)
public void ExceptWith(IEnumerable<T> other)
{
- throw new NotImplementedException();
+ if (ReferenceEquals(this, other))
+ {
+ Clear();
+ return;
+ }
+
+ foreach (var item in other)
+ {
+ TryRemove(item);
Review Comment:
Please see the optimizations that Microsoft added at:
https://github.com/dotnet/runtime/blob/7ce5d0fdf728bb2ae213541b17c2e489d2b06d09/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/HashSet.cs#L501.
We can probably improve this implementation.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
