On Wed, 14 Jun 2023 11:14:02 GMT, Jaikiran Pai <[email protected]> wrote:
>> Daniel Fuchs has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains two additional
>> commits since the last revision:
>>
>> - Merge branch 'master' into TimeSource
>> - 8309939
>
> src/java.net.http/share/classes/jdk/internal/net/http/ConnectionPool.java
> line 132:
>
>> 130: }
>> 131:
>> 132: ConnectionPool(long clientId, TimeSource timeSource) {
>
> Hello Daniel, as far as I can see, no one calls this constructor outside of
> this very class and this seems to be always passed a constant
> `TimeSource.source()`. So maybe we don't have to have a `timeSource` field in
> this class and instead just use `TimeSource.now()` at call sites within this
> class (and `ExpiryList`) whenever we want `now()`?
This constructor should in fact take an InstantSource, not a TimeSource. It was
added in case we wanted to upgrade the whitebox ConnectionPoolTest to create an
instance of ConnectionPool with a custom InstantSource.
> src/java.net.http/share/classes/jdk/internal/net/http/common/TimeSource.java
> line 76:
>
>> 74: // The use of Integer.MAX_VALUE is arbitrary.
>> 75: // Any value not too close to Long.MAX_VALUE
>> 76: // would do.
>
> The `Integer.MAX_VALUE` gets used in `isInWindow()` method implementation,
> through the use of `TIME_WINDOW` member. Should this comment about
> `Integer.MAX_VALUE` instead be moved as a field comment for `TIME_WINDOW`?
> That way it's closer to where the `Integer.MAX_VALUE` is actually used.
Done
> src/java.net.http/share/classes/jdk/internal/net/http/common/TimeSource.java
> line 82:
>
>> 80: }
>> 81:
>> 82: // @ForceInline
>
> Should these couple of commented out `@ForceInline` be deleted?
Done
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14450#discussion_r1229539076
PR Review Comment: https://git.openjdk.org/jdk/pull/14450#discussion_r1229540292
PR Review Comment: https://git.openjdk.org/jdk/pull/14450#discussion_r1229540099
