Thanks for the suggestion Daniel, looks much nicer without the casting
in the immutableCopy function as well!
I made a new patch with those reverted changes and it looks much nicer
and still behaves as expected.
webrev:
http://cr.openjdk.java.net/~ccleary/issues/webrevs-store/8246047/webrevs/webrev.03/
Thanks!
Conor
On 14/08/2020 15:49, Daniel Fuchs wrote:
Hi Conor,
Thanks for addressing this technical debt.
I don't think this is correct. You transformed the immutable
copy at line 137 into a shallow clone, since now both copies
share the same mutable list.
I suggest to revert the changes at lines:
66,67,144 and 145.
(and that will be much simpler in the bargain :-) )
best regards,
-- daniel
On 14/08/2020 15:37, Conor Cleary wrote:
Hi all,
Requesting some reviewers for a patch concerning JDK-8246047 'Replace
LinkedList impl in net.http.websocket.BuilderImpl'. This patch
replaces LinkedList data structures used in the net.http.websocket
BuilderImpl class with ArrayLists. In particular, the 'headers' and
'subprotocols' Collections in the class are now assigned ArrayLists
in the BuilderImpl constructors.
Some justifications for this change are as follows:
* Sequential Access Times for ArrayLists are improved due to locality
of reference (i.e ArrayList elements stored in same neighbourhood)
* Get(index) operations are O(1) time complexity for ArrayLists as
opposed to worst-case O(N-1) for LinkedLists
* While insertion operations can be expensive (O(N) in the worst
case), the 'headers' and 'subprotocols' lists are not modified after
creation and so this is of minor concern
Additional justifications or challenges to them are welcome! The
general idea is that ArrayLists out-perform LinkedLists in this
particular scenario.
* webrev:
http://cr.openjdk.java.net/~ccleary/issues/webrevs-store/8246047/webrevs/webrev.02/
* bug: https://bugs.openjdk.java.net/browse/JDK-8246047
Conor
