Re: RFR: 8245194: Unix domain socket channel implementation [v35]
On Thu, 22 Oct 2020 08:14:31 GMT, Michael McMahon wrote: >> Continuing this review as a PR on github with the comments below >> incorporated. I expect there will be a few more iterations before >> integrating. >> >> On 06/09/2020 19:47, Alan Bateman wrote: >>> On 26/08/2020 15:24, Michael McMahon wrote: As I mentioned the other day, I wasn't able to use the suggested method on Windows which returns an absolute path. So, I added a method to WindowsPath which returns the path in the expected UTF-8 encoding as a byte[]. Let me know what you think of that. There is a fair bit of other refactoring and simplification done also. Next version is at: http://cr.openjdk.java.net/~michaelm/8245194/impl.webrev/webrev.9/ >>> Adding a method to WindowsPath to encode the path using UTF-8 is okay but I >>> don't think we should be caching it as the encoding for sun_path is an >>> outlier on Windows. I'm also a bit dubious about encoding a relative path >>> when the resolved path (before encoding) is > 247 chars. The documentation >>> on the MS site isn't very completely and I think there are a number points >>> that require clarification to fully understand how this will work with >>> relative, directly relative and drive relative paths. >>> >> >> Maybe it would be better to just use the path returned from toString() and >> do the conversion to UTF-8 externally. That would leave WindowsPath >> unchanged. >> >>> In the same area, the new PathUtil is a bit inconsistent with the existing >>> provider code. One suggestion is to add a method to >>> AbstractFileSystemProvider instead. That is the base class the platform >>> providers and would be a better place to get the file path in bytes. >>> >> >> Okay, I gave the method a name that is specific to Unix domain sockets >> because this UTF-8 strangeness is not likely to be useful by other >> components. >> >>> One other comment on the changes to the file system provider it should be >>> okay to change UnixUserPrinipals ad its fromUid and fromGid method to be >>> public. This would mean that the patch shouldn't need to add UnixUserGroup >>> (the main issue is that class is that it means we end up with two classes >>> with static factory methods doing the same thing). >> >> Okay, that does simplify it a bit. >> >> Thanks, >> Michael. >> >>> -Alan. >>> >>> >>> >>> >>> >>> > > Michael McMahon has updated the pull request incrementally with one > additional commit since the last revision: > > - fixed 32 bit Linux build error > - test updates from Chris src/java.base/windows/classes/sun/nio/ch/WindowsSelectorImpl.java line 148: > 146: // Disable the Nagle algorithm so that the wakeup is more > immediate > 147: SinkChannelImpl sink = (SinkChannelImpl)wakeupPipe.sink(); > 148: wakeupSinkFd = ((SelChImpl)sink).getFDVal(); The "Disable the ..." comment can be removed as it's handled in PipeImpl now. "sink" can be removed so that wakeupSourceFd and wakeupSinkFd can be created the same way. - PR: https://git.openjdk.java.net/jdk/pull/52
Re: RFR: 8255299: Drop explicit zeroing at instantiation of Atomic* objects
On Thu, 22 Oct 2020 20:46:15 GMT, Сергей Цыпанов
wrote:
> As discussed in https://github.com/openjdk/jdk/pull/510 there is never a
> reason to explicitly instantiate any instance of `Atomic*` class with its
> default value, i.e. `new AtomicInteger(0)` could be replaced with `new
> AtomicInteger()` which is faster:
> @State(Scope.Thread)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @BenchmarkMode(value = Mode.AverageTime)
> public class AtomicBenchmark {
> @Benchmark
> public Object defaultValue() {
> return new AtomicInteger();
> }
> @Benchmark
> public Object explicitValue() {
> return new AtomicInteger(0);
> }
> }
> THis benchmark demonstrates that `explicitValue()` is much slower:
> Benchmark Mode Cnt Score Error Units
> AtomicBenchmark.defaultValue avgt 30 4.778 ± 0.403 ns/op
> AtomicBenchmark.explicitValue avgt 30 11.846 ± 0.273 ns/op
> So meanwhile https://bugs.openjdk.java.net/browse/JDK-8145948 is still in
> progress we could trivially replace explicit zeroing with default
> constructors gaining some performance benefit with no risk.
>
> I've tested the changes locally, both tier1 and tier 2 are ok.
>
> Could one create an issue for tracking this?
client changes are fine
-
Marked as reviewed by prr (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/818
Re: RFR: 8255078: sun/net/ftp/imp/FtpClient$MLSxParser uses wrong datetime format [v3]
On Fri, 23 Oct 2020 17:50:51 GMT, Igor Ignatyev wrote:
>> Hi all,
>>
>> could you please review this small patch?
>>
>> according to [RFC3659](https://tools.ietf.org/html/rfc3659), time values in
>> MLSx response have the following format:
>>>time-val = 14DIGIT [ "." 1*DIGIT ]
>>>
>>>The leading, mandatory, fourteen digits are to be interpreted as, in
>>>order from the leftmost, four digits giving the year, with a range of
>>>1000--, two digits giving the month of the year, with a range of
>>>01--12, two digits giving the day of the month, with a range of
>>>01--31, two digits giving the hour of the day, with a range of
>>>00--23, two digits giving minutes past the hour, with a range of
>>>00--59, and finally, two digits giving seconds past the minute, with
>>>a range of 00--60 (with 60 being used only at a leap second). Years
>>>in the tenth century, and earlier, cannot be expressed. This is not
>>>considered a serious defect of the protocol.
>>>
>>>The optional digits, which are preceded by a period, give decimal
>>>fractions of a second. These may be given to whatever precision is
>>>appropriate to the circumstance, however implementations MUST NOT add
>>>precision to time-vals where that precision does not exist in the
>>>underlying value being transmitted.
>>>
>>>Symbolically, a time-val may be viewed as
>>>
>>> MMDDHHMMSS.sss
>>>
>>>The "." and subsequent digits ("sss") are optional. However the "."
>>>MUST NOT appear unless at least one following digit also appears.
>>>
>>
>> `MLSxParser`, however, uses `SimpleDateFormat("MMddhhmmss")` (which is
>> wrong b/c it uses `hh` instead of `HH` and doesn't account for optional
>> `.sss`) to parse modify/create facts and ignore any parse exceptions.
>>
>> `FtpClient` actually already has and uses `SimpleDateFormat` w/ right
>> formats in `getLastModified` where it parses MDTM response, the patch
>> refactors the code to reuse the same `SimpleDateFormat` in `MLSxParser`.
>>
>> testing:
>> * [x] tier1
>> * [x] `test/jdk/sun/net` on `{linux,windows,macosx}-x64`
>
> Igor Ignatyev has updated the pull request incrementally with one additional
> commit since the last revision:
>
> use only filename in assert message
src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 1777:
> 1775: }
> 1776:
> 1777: private static Date parseRfc3659TimeValue(String s) {
Shouldn't this method be synchronized because `SimpleDateFormat` is not
thread-safe, but instances are stored in the static field `dateFormats`?
(Note though that this issue existed before as well)
-
PR: https://git.openjdk.java.net/jdk/pull/776
