RFR: 8293067: (fs) Implement WatchService using system library (macOS) [v3]

[Brian Burkhalter]

On Thu, 22 Sep 2022 07:06:46 GMT, Maxim Kartashev <mkartashev at openjdk.org> wrote:

>> src/java.base/macosx/classes/sun/nio/fs/MacOSXWatchService.java line 399:
>> 
>>> 397:                 }
>>> 398: 
>>> 399:                 final int flags = unsafe.getInt(eventFlagsPtr);
>> 
>> This looks like the only use of `Unsafe`. Could the objective be accomplished without using it, e.g., changing `long eventFlagsPtr` to a `long[]` or, maybe even simpler, a `boolean[]`?
>
> Earlier on the mailing list @AlanBateman wrote "It would help the review if the native code is just simple wrappers about the FSEvents as we'll likely replace a lot of the JNI code in this area in time." 
> I understood that as a suggestion to keep the native code as simple as possible and, conversely, do as much as possible on the Java side. Reading native memory here was one of the decisions that were based on that understanding. 
> I may have, of course, read too much into it, so I'm not opposed to implementing your suggestion. I would like, however, to have a confirmation that this direction of the changes is OK with @AlanBateman also.

Your understanding is correct, but sometimes precisely mapping the native API can result in more complication than needed. While this might not be a strictly analogous example, consider the BSD system call `setattrlist(2)`. We only need to use it at present to set file times, so this call was exposed at the Java level with a simpler set of parameters. See [sun.nio.fs.BsdNativeDispatcher::setattrlist](https://github.com/openjdk/jdk/blob/5285035ed9bb43a40108e4d046e0de317730f193/src/java.base/macosx/classes/sun/nio/fs/BsdNativeDispatcher.java#L84
). I’ll leave it to @AlanBateman to express his own opinion on this.

>> src/java.base/macosx/native/libnio/fs/MacOSXWatchService.c line 73:
>> 
>>> 71:         (*env)->CallVoidMethod(env, watchService, callbackMID,
>>> 72:                                streamRef, javaEventPathsArray, eventFlags);
>>> 73:     }
>> 
>> Is there a way to notify of events without an upcall? Maybe add the necessary values to a queue of some sort observed by the Java layer?
>
> I'm not sure I know how to synchronize native code with Java without any upcall to the Java side; but maybe you are suggesting something as simple as a `notify()` call in place of the current callback with many arguments?.
> 
> And also this goes against my understanding that the native part has to be kept as simple and as straightforward as possible. Having it  maintain a queue that has to be synchronized with Java will certainly complicate the native part.

I was thinking of maybe something like a [BlockingQueue](https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/util/concurrent/BlockingQueue.html) field on `sun.nio.fs.MacOSXWatchService`, but I am not sure whether this is any more correct.

If the callback is retained, it might be worth considering using [JNU_CallMethodByName](https://github.com/openjdk/jdk/blob/5285035ed9bb43a40108e4d046e0de317730f193/src/java.base/share/native/libjava/jni_util.h#L134).

>> src/java.base/macosx/native/libnio/fs/MacOSXWatchService.c line 89:
>> 
>>> 87: {
>>> 88:     JNIEnv *env = (JNIEnv *) JNU_GetEnv(jvm, JNI_VERSION_1_2);
>>> 89:     if (!env) { // Shouldn't happen as run loop starts from Java code
>> 
>> Is this check, hence the `jvm` extern, really necessary?
>
> The check isn't strictly necessary, so I'm goign to drop it (there's a JNI version check in `nio_util.c`), but the `jvm` extern seems necessary because that's how `JNIEnv` is obtained and the latter is necessary to communicate the events back to the Java side.

I don’t know whether it’s possible to get rid of the `jvm` extern, i.e. , whether there’s another way to get the `JNIEnv`.

>> src/java.base/macosx/native/libnio/fs/MacOSXWatchService.c line 199:
>> 
>>> 197: 
>>> 198:     FSEventStreamStop(streamRef);       // Unregister with the FS Events service.
>>> 199:                                         // No more callbacks from this stream.
>> 
>> Does `FSEventStreamUnscheduleFromRunLoop()` need to be called here?
>
> I think not. The [documentation](https://developer.apple.com/documentation/coreservices/1446990-fseventstreaminvalidate?language=objc) for `FSEventStreamInvalidate()` that gets called immediately after this line states the following:
>> Invalidates the stream, like CFRunLoopSourceInvalidate() does for a CFRunLoopSourceRef. It will be unscheduled  from any runloops or dispatch queues upon which it had been scheduled.
> 
> So calling `FSEventStreamUnscheduleFromRunLoop()` seems superfluous; its semantics is described as
>> ... removes the stream from the specified run loop, like CFRunLoopRemoveSource() does for a CFRunLoopSourceRef.

That sounds correct.

-------------

PR: https://git.openjdk.org/jdk/pull/10140