[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-nio-dev
Subject: Re: JDK-8259873: (fs) Add java.nio.file.attribute.BasicWithKeyFileAttributeView interface
From: Alan Bateman <Alan.Bateman () oracle ! com>
Date: 2021-01-31 11:21:56
Message-ID: 3d8fca61-ba0f-a47c-0944-07e9a78a22d6 () oracle ! com
[Download RAW message or body]
On 19/01/2021 22:36, Renaud Paquay wrote:
> :
>
> PR 2122
> =======
>
> PR 2122 is an "in place" (i.e. no behavior or public API change)
> implementation change in WindowsDirectoryStream: Instead of using
> FindFirstFile
> <https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-findfirstfilew>,FindNextFile
> <https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-findnextfilew>,
> the new implementation uses NtQueryDirectoryFile
> <https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/nf-ntifs-ntquerydirectoryfile>,
> which is more efficient at enumerating directory entries and allows
> acquiring file keys for "free" with each entry.
>
I've looked through the changes in pull/2122.
This changes the implementation from using
FindFirst/FindNextFile/FindClose to low-level DDK functions. These seem
to be documented so moving to these functions may be okay. It would be
useful to get some performance data with just this change if possible.
Also it would be useful to get confirmation that there aren't any issues
with using UNCs or mounted volumes as we've had interop problems in the
past with SAMBA and some CIFS servers.
Adding ntifs_min.h is probably okay but would be useful to know if
ntifs.h is the DDK only, are there are any VC++ or SDK releases that
have this include file? We'll probably need to put the constants into
WindowsConstants so they can be used by WindowsDirectoryStream.
There will be cleanup needed to get the patch consistent with the
existing code. One issue is that the native functions that
WindowsNativeDispatcher defines should map, where possible, to call on
win32 function. This means that OpenNtQueryDirectoryInformation needs to
be split up so that WindowsDirectoryStream calls CreateFile and then
NtQueryDirectoryInformation with the file handle. It should be okay for
WindowsDirectoryStream to work with NTSTATUS codes and then call
RtlNtStatusToDosError to translate to system errors when required.
Adding fromFieldIdFullDirInformation to WindowsFileAttributes is okay
although supplementing it with the volume serial number is a bit odd.
The other other methods (getFileNameFromFieldIdFullDirInformation and
getNextOffsetFromFieldIdDirInformation) are nothing to do with
WindowsFileAttributes so shouldn't be there. WindowsDirectoryStream may
be a better place to operation on FieldIdFullDirInformation structure or
else introduces a new class to encapsulate an instance. This will a lot
cleaner in the future when we have Panama.
It would also be good if you could try to keep the style, naming, and
line lengths consistent if you can. Some of the really long 140+
character lines will make future reviewing of side-by-side diffs hard to
review.
-Alan
[Attachment #3 (text/html)]
<html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
On 19/01/2021 22:36, Renaud Paquay wrote:<br>
<blockquote type="cite" \
cite="mid:CAFcJ+g6pL_3fUaCAQU+59QcOVSaGc-HrcyBEAW4hjpVvL0OGoQ@mail.gmail.com">
<div dir="ltr">:
<div><br>
</div>
<div>PR 2122<br>
</div>
<div>=======</div>
<div><br>
</div>
<div>PR 2122 is an "in place" (i.e. no behavior or public API
change) implementation change in WindowsDirectoryStream:
Instead of using <a \
href="https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-findfirstfilew" \
moz-do-not-send="true">FindFirstFile</a>,<a \
href="https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-findnextfilew" \
moz-do-not-send="true">FindNextFile</a>, the new implementation uses <a \
href="https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/nf-ntifs-ntquerydirectoryfile" \
moz-do-not-send="true">NtQueryDirectoryFile</a>, which is more efficient at \
enumerating directory entries and allows acquiring file keys for "free" \
with each entry.</div> <br>
</div>
</blockquote>
I've looked through the changes in pull/2122.<br>
<br>
This changes the implementation from using
FindFirst/FindNextFile/FindClose to low-level DDK functions. These
seem to be documented so moving to these functions may be okay. It
would be useful to get some performance data with just this change
if possible. Also it would be useful to get confirmation that there
aren't any issues with using UNCs or mounted volumes as we've had
interop problems in the past with SAMBA and some CIFS servers.<br>
<br>
Adding ntifs_min.h is probably okay but would be useful to know if
ntifs.h is the DDK only, are there are any VC++ or SDK releases that
have this include file? We'll probably need to put the constants
into WindowsConstants so they can be used by WindowsDirectoryStream.<br>
<br>
There will be cleanup needed to get the patch consistent with the
existing code. One issue is that the native functions that
WindowsNativeDispatcher defines should map, where possible, to call
on win32 function. This means that OpenNtQueryDirectoryInformation
needs to be split up so that WindowsDirectoryStream calls CreateFile
and then NtQueryDirectoryInformation with the file handle. It should
be okay for WindowsDirectoryStream to work with NTSTATUS codes and
then call RtlNtStatusToDosError to translate to system errors when
required.<br>
<br>
Adding fromFieldIdFullDirInformation to WindowsFileAttributes is
okay although supplementing it with the volume serial number is a
bit odd. The other other methods
(getFileNameFromFieldIdFullDirInformation and
getNextOffsetFromFieldIdDirInformation) are nothing to do with
WindowsFileAttributes so shouldn't be there. WindowsDirectoryStream
may be a better place to operation on FieldIdFullDirInformation
structure or else introduces a new class to encapsulate an instance.
This will a lot cleaner in the future when we have Panama.<br>
<br>
It would also be good if you could try to keep the style, naming,
and line lengths consistent if you can. Some of the really long 140+
character lines will make future reviewing of side-by-side diffs
hard to review.<br>
<br>
-Alan<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic