Use a single array in SelectedSelectionKeySet #6083

Scottmitch · 2016-12-01T06:57:41Z

Motivation:
SelectedSelectionKeySet currently uses 2 arrays internally and users are expected to call flip() to access the underlying array and switch the active array. However we do not concurrently use 2 arrays at the same time and we can get away with using a single array if we are careful about when we reset the elements of the array.

Modifications:

Introduce SelectedSelectionKeySetSelector which wraps a Selector and ensures we reset the underlying SelectedSelectionKeySet data structures before we select
The loop bounds in NioEventLoop#processSelectedKeysOptimized can be defined more precisely because we know the real size of the underlying array

Result:
Fixes #6058

Scottmitch · 2016-12-01T06:58:01Z

@lightningMan @normanmaurer - FYI

lightningMan · 2016-12-02T01:43:42Z

pong… Scott Mitchell <notifications@github.com>于2016年12月1日周四14:58写道：

…

@lightningMan <https://github.com/lightningMan> @normanmaurer <https://github.com/normanmaurer> - FYI — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6083 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABmkeiXPSx3dLkUegyvp0EEFdenc_ZNvks5rDnAKgaJpZM4LBEBM> .

johnou · 2016-12-02T06:20:45Z

@lightningMan Scott is letting you know that he refactored SelectedSelectionKeySet, you can review the code here https://github.com/netty/netty/pull/6083/files

lightningMan · 2016-12-02T07:05:19Z

@johnou thank you for your friendly reminder, I have reviewed the code ^^

normanmaurer · 2016-12-08T19:03:26Z

@Scottmitch I am still not 100 % convinced we need these change and I am a bit concerned about introduce a regression as we also have no tests for this class (as we had none before as well).

So let me cut a release first without this and then we can pick this one up and see... Ok ?

Scottmitch · 2016-12-08T19:07:47Z

Yip I'm fine with delaying this until 4.1.8.

"Need" is a loaded statement :) Things work today so I guess in that sense I agree, however this simplifies and removes complexity that doesn't seem useful. SelectedSelectionKeySetSelector.java introduced in this PR ensures that we reset the array backing the Set at the appropriate time, and so in theory this should help with correctness (unless there are bugs in my implementation). Bounding the iteration and removing duplicate null checks may also clarify functionality.

normanmaurer · 2016-12-08T19:09:09Z

@Scottmitch haha fair enough :) So let us pick this up as first thing after the release 👍

normanmaurer · 2017-02-10T15:37:28Z

@Scottmitch if you want to pull this in can you please at least add a testcase for the Set impl ?

Scottmitch · 2017-02-11T03:36:12Z

@normanmaurer - Fair enough. I added a few unit tests to verify expected behavior. Let me know if you want more.

johnou · 2017-02-11T11:18:28Z

transport/src/main/java/io/netty/channel/nio/NioEventLoop.java

-                    }
-                    selectedKeys[i] = null;
-                }
+                this.selectedKeys.reset(i + 1);


no null check and break while iterating in reset, intentional?

Yes. We know the explicit size of the array and we check for null on insertion into the set.

right but the previous impl stopped iterating on the first non null element found presumably to avoid iterating the full set and burn less cpu cycles?

The current code and the previous code should be logically equivalent but have the following differences:

the previous implementation didn't utilize the size of the set when iterating. We now have a fixed/known boundary to iterate to (size) instead of conditional based on the next element contents.

This set doesn't allow null elements to be added, so checking each element for null is not necessary.

I pushed some updated code to make the relationship with this.selectedKeys more clear.

johnou · 2017-02-11T11:21:28Z

transport/src/main/java/io/netty/channel/nio/SelectedSelectionKeySet.java

+        for (; start < size; ++start) {
+            keys[start] = null;
+        }
+        size = 0;


does setting size to 0 when start is non 0 make sense?

I think so. As we iterate over the array in NioEventLoop we null entries out. This method is used if we decide we are done iterating over the array and want to make the set ready for re-use. Do you see an issue here?

no issue comes to mind at the moment, just unexpected that size of the set might be 0 when there are non null entries in the set.

general use of this methods may result in non null entries being in the set when the size is zero, but keep in mind this class is package private and so is this method. This shouldn't happen in practice based upon the narrow use case of this class (as we iterate we null elements out, and if we decide to prematurely stop iteration we call this method to null out the remaining elements #6083 (comment)).

normanmaurer · 2017-02-16T19:44:06Z

transport/src/test/java/io/netty/channel/nio/SelectedSelectionKeySetTest.java

@@ -0,0 +1,66 @@
+/*
+ * Copyright 2013 The Netty Project


normanmaurer · 2017-02-16T19:49:09Z

transport/src/main/java/io/netty/channel/nio/SelectedSelectionKeySet.java

+    void reset(int start) {
+        for (; start < size; ++start) {
+            keys[start] = null;
+        }


Use Arrays.fill(...) ?

normanmaurer · 2017-02-16T19:49:54Z

transport/src/main/java/io/netty/channel/nio/SelectedSelectionKeySetSelector.java

@@ -0,0 +1,80 @@
+/*
+ * Copyright 2016 The Netty Project


Motivation: SelectedSelectionKeySet currently uses 2 arrays internally and users are expected to call flip() to access the underlying array and switch the active array. However we do not concurrently use 2 arrays at the same time and we can get away with using a single array if we are careful about when we reset the elements of the array. Modifications: - Introduce SelectedSelectionKeySetSelector which wraps a Selector and ensures we reset the underlying SelectedSelectionKeySet data structures before we select - The loop bounds in NioEventLoop#processSelectedKeysOptimized can be defined more precisely because we know the real size of the underlying array Result: Fixes netty#6058

Scottmitch · 2017-02-16T19:54:26Z

@normanmaurer - comments addressed PTAL

Scottmitch · 2017-02-16T23:11:03Z

4.1 (795f318) 4.0 (26124f6)

Scottmitch self-assigned this Dec 1, 2016

lightningMan approved these changes Dec 2, 2016

View reviewed changes

Scottmitch requested a review from normanmaurer December 8, 2016 18:59

Scottmitch force-pushed the nio_optimized_improve branch from 9a5f901 to 0d826a9 Compare January 3, 2017 21:02

Scottmitch force-pushed the nio_optimized_improve branch from 0d826a9 to 320437d Compare February 11, 2017 03:35

johnou reviewed Feb 11, 2017

View reviewed changes

Scottmitch force-pushed the nio_optimized_improve branch 2 times, most recently from 8c87137 to ee85e88 Compare February 13, 2017 01:39

normanmaurer requested changes Feb 16, 2017

View reviewed changes

Scottmitch force-pushed the nio_optimized_improve branch from ee85e88 to dbd2fa0 Compare February 16, 2017 19:54

normanmaurer approved these changes Feb 16, 2017

View reviewed changes

Scottmitch closed this Feb 16, 2017

Scottmitch deleted the nio_optimized_improve branch February 16, 2017 23:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a single array in SelectedSelectionKeySet #6083

Use a single array in SelectedSelectionKeySet #6083

Scottmitch commented Dec 1, 2016

Scottmitch commented Dec 1, 2016

lightningMan commented Dec 2, 2016 via email

johnou commented Dec 2, 2016

lightningMan commented Dec 2, 2016 •

edited

Loading

normanmaurer commented Dec 8, 2016

Scottmitch commented Dec 8, 2016

normanmaurer commented Dec 8, 2016

normanmaurer commented Feb 10, 2017

Scottmitch commented Feb 11, 2017

johnou Feb 11, 2017

Scottmitch Feb 11, 2017

johnou Feb 12, 2017

Scottmitch Feb 13, 2017

johnou Feb 11, 2017

Scottmitch Feb 11, 2017

johnou Feb 12, 2017 •

edited

Loading

Scottmitch Feb 13, 2017 •

edited

Loading

normanmaurer Feb 16, 2017

normanmaurer Feb 16, 2017

normanmaurer Feb 16, 2017

Scottmitch commented Feb 16, 2017

Scottmitch commented Feb 16, 2017

Use a single array in SelectedSelectionKeySet #6083

Use a single array in SelectedSelectionKeySet #6083

Conversation

Scottmitch commented Dec 1, 2016

Scottmitch commented Dec 1, 2016

lightningMan commented Dec 2, 2016 via email

johnou commented Dec 2, 2016

lightningMan commented Dec 2, 2016 • edited Loading

normanmaurer commented Dec 8, 2016

Scottmitch commented Dec 8, 2016

normanmaurer commented Dec 8, 2016

normanmaurer commented Feb 10, 2017

Scottmitch commented Feb 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnou Feb 12, 2017 • edited Loading

Choose a reason for hiding this comment

Scottmitch Feb 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Scottmitch commented Feb 16, 2017

Scottmitch commented Feb 16, 2017

lightningMan commented Dec 2, 2016 •

edited

Loading

johnou Feb 12, 2017 •

edited

Loading

Scottmitch Feb 13, 2017 •

edited

Loading