You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2022/05/27 07:46:38 UTC
[GitHub] [ignite] SammyVimes opened a new pull request, #10045: IGNITE-17043 Shrink GridHandleTable on clear
SammyVimes opened a new pull request, #10045:
URL: https://github.com/apache/ignite/pull/10045
https://issues.apache.org/jira/browse/IGNITE-17043
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
### The Contribution Checklist
- [ ] There is a single JIRA ticket related to the pull request.
- [ ] The web-link to the pull request is attached to the JIRA ticket.
- [ ] The JIRA ticket has the _Patch Available_ state.
- [ ] The pull request body describes changes that have been made.
The description explains _WHAT_ and _WHY_ was made instead of _HOW_.
- [ ] The pull request title is treated as the final commit message.
The following pattern must be used: `IGNITE-XXXX Change summary` where `XXXX` - number of JIRA issue.
- [ ] A reviewer has been mentioned through the JIRA comments
(see [the Maintainers list](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-ReviewProcessandMaintainers))
- [ ] The pull request has been checked by the Teamcity Bot and
the `green visa` attached to the JIRA ticket (see [TC.Bot: Check PR](https://mtcga.gridgain.com/prs.html))
### Notes
- [How to Contribute](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute)
- [Coding abbreviation rules](https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules)
- [Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines)
- [Apache Ignite Teamcity Bot](https://cwiki.apache.org/confluence/display/IGNITE/Apache+Ignite+Teamcity+Bot)
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com _#ignite_ channel.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] sashapolo commented on a diff in pull request #10045: IGNITE-17043 Shrink GridHandleTable on clear
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #10045:
URL: https://github.com/apache/ignite/pull/10045#discussion_r883535106
##########
modules/core/src/main/java/org/apache/ignite/internal/util/GridHandleTable.java:
##########
@@ -181,6 +203,32 @@ private void growEntries() {
objs = newObjs;
}
+ /**
+ * Tries to gradually shrink hash table by factor of two when it's cleared.
+ *
+ * @return {@code true} if shrinked the table, {@code false} otherwise.
+ */
+ private boolean shrink() {
+ int newLen = objs.length;
+
+ if (newLen > initCap) {
+ int shrinked = (newLen - 1) / 2;
Review Comment:
`shirnked` is incorrect, should be `shrank`
##########
modules/core/src/main/java/org/apache/ignite/internal/util/GridHandleTable.java:
##########
@@ -181,6 +203,32 @@ private void growEntries() {
objs = newObjs;
}
+ /**
+ * Tries to gradually shrink hash table by factor of two when it's cleared.
+ *
+ * @return {@code true} if shrinked the table, {@code false} otherwise.
+ */
+ private boolean shrink() {
+ int newLen = objs.length;
+
+ if (newLen > initCap) {
+ int shrinked = (newLen - 1) / 2;
Review Comment:
why `newLen - 1`?
##########
modules/core/src/main/java/org/apache/ignite/internal/util/GridHandleTable.java:
##########
@@ -181,6 +203,32 @@ private void growEntries() {
objs = newObjs;
}
+ /**
+ * Tries to gradually shrink hash table by factor of two when it's cleared.
+ *
+ * @return {@code true} if shrinked the table, {@code false} otherwise.
+ */
+ private boolean shrink() {
+ int newLen = objs.length;
+
+ if (newLen > initCap) {
+ int shrinked = (newLen - 1) / 2;
Review Comment:
I think this code can actually be written a little bit simpler:
```
int newLen = Math.max(objs.length / 2, initCap);
if (newLen >= size && newLen < objs.length) {
init(newLen);
return true;
}
return false;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] tkalkirill commented on a diff in pull request #10045: IGNITE-17043 Shrink GridHandleTable on clear
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #10045:
URL: https://github.com/apache/ignite/pull/10045#discussion_r883392640
##########
modules/core/src/main/java/org/apache/ignite/internal/util/GridHandleTable.java:
##########
@@ -107,6 +120,10 @@ public int lookup(Object obj) {
* Resets table to its initial (empty) state.
*/
public void clear() {
+ if (size < objs.length) {
+ shrink();
Review Comment:
```suggestion
shrink();
retrun;
```
##########
modules/core/src/main/java/org/apache/ignite/internal/util/GridHandleTable.java:
##########
@@ -181,6 +198,24 @@ private void growEntries() {
objs = newObjs;
}
+ /**
+ * Tries to gradually shrink hash table by factor of two when it's cleared.
+ */
+ private void shrink() {
+ int newLen = objs.length;
+
+ if (newLen > initCap) {
+ int shrinked = (newLen - 1) / 2;
+ if (shrinked >= size)
+ newLen = shrinked;
Review Comment:
```suggestion
int shrinked = newLen / 2;
if (shrinked >= size)
newLen = shrinked;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] SammyVimes commented on a diff in pull request #10045: IGNITE-17043 Shrink GridHandleTable on clear
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #10045:
URL: https://github.com/apache/ignite/pull/10045#discussion_r883552210
##########
modules/core/src/main/java/org/apache/ignite/internal/util/GridHandleTable.java:
##########
@@ -107,6 +121,14 @@ public int lookup(Object obj) {
* Resets table to its initial (empty) state.
*/
public void clear() {
+ if (size < objs.length) {
+ if (shrink()) {
+ size = 0;
+
+ return;
+ }
+ }
+
System.arraycopy(spineEmpty, 0, spine, 0, spineEmpty.length);
System.arraycopy(nextEmpty, 0, next, 0, nextEmpty.length);
Review Comment:
Sounds legit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] macrergate commented on a diff in pull request #10045: IGNITE-17043 Shrink GridHandleTable on clear
Posted by GitBox <gi...@apache.org>.
macrergate commented on code in PR #10045:
URL: https://github.com/apache/ignite/pull/10045#discussion_r883460225
##########
modules/core/src/main/java/org/apache/ignite/internal/util/GridHandleTable.java:
##########
@@ -107,6 +121,14 @@ public int lookup(Object obj) {
* Resets table to its initial (empty) state.
*/
public void clear() {
+ if (size < objs.length) {
+ if (shrink()) {
+ size = 0;
+
+ return;
+ }
+ }
+
System.arraycopy(spineEmpty, 0, spine, 0, spineEmpty.length);
System.arraycopy(nextEmpty, 0, next, 0, nextEmpty.length);
Review Comment:
How about clearing not all nextEmpty and objs array, but only first size elements?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] macrergate commented on a diff in pull request #10045: IGNITE-17043 Shrink GridHandleTable on clear
Posted by GitBox <gi...@apache.org>.
macrergate commented on code in PR #10045:
URL: https://github.com/apache/ignite/pull/10045#discussion_r883460225
##########
modules/core/src/main/java/org/apache/ignite/internal/util/GridHandleTable.java:
##########
@@ -107,6 +121,14 @@ public int lookup(Object obj) {
* Resets table to its initial (empty) state.
*/
public void clear() {
+ if (size < objs.length) {
+ if (shrink()) {
+ size = 0;
+
+ return;
+ }
+ }
+
System.arraycopy(spineEmpty, 0, spine, 0, spineEmpty.length);
System.arraycopy(nextEmpty, 0, next, 0, nextEmpty.length);
Review Comment:
How about clearing not all next and objs array, but only first size elements?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] SammyVimes commented on a diff in pull request #10045: IGNITE-17043 Shrink GridHandleTable on clear
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #10045:
URL: https://github.com/apache/ignite/pull/10045#discussion_r883584960
##########
modules/core/src/main/java/org/apache/ignite/internal/util/GridHandleTable.java:
##########
@@ -107,6 +121,14 @@ public int lookup(Object obj) {
* Resets table to its initial (empty) state.
*/
public void clear() {
+ if (size < objs.length) {
+ if (shrink()) {
+ size = 0;
+
+ return;
+ }
+ }
+
System.arraycopy(spineEmpty, 0, spine, 0, spineEmpty.length);
System.arraycopy(nextEmpty, 0, next, 0, nextEmpty.length);
Review Comment:
On the other hand, objs should be cleared entirely to avoid referencing objects that are no longer needed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] SammyVimes commented on a diff in pull request #10045: IGNITE-17043 Shrink GridHandleTable on clear
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #10045:
URL: https://github.com/apache/ignite/pull/10045#discussion_r883399097
##########
modules/core/src/main/java/org/apache/ignite/internal/util/GridHandleTable.java:
##########
@@ -107,6 +120,10 @@ public int lookup(Object obj) {
* Resets table to its initial (empty) state.
*/
public void clear() {
+ if (size < objs.length) {
+ shrink();
Review Comment:
We still have to perform the rest of the "clear", because it's basically the same as in constructor + potentially we might not shrink at all
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] sashapolo commented on a diff in pull request #10045: IGNITE-17043 Shrink GridHandleTable on clear
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #10045:
URL: https://github.com/apache/ignite/pull/10045#discussion_r883535106
##########
modules/core/src/main/java/org/apache/ignite/internal/util/GridHandleTable.java:
##########
@@ -181,6 +203,32 @@ private void growEntries() {
objs = newObjs;
}
+ /**
+ * Tries to gradually shrink hash table by factor of two when it's cleared.
+ *
+ * @return {@code true} if shrinked the table, {@code false} otherwise.
+ */
+ private boolean shrink() {
+ int newLen = objs.length;
+
+ if (newLen > initCap) {
+ int shrinked = (newLen - 1) / 2;
Review Comment:
`shrinked` is incorrect, should be `shrank`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] SammyVimes commented on a diff in pull request #10045: IGNITE-17043 Shrink GridHandleTable on clear
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #10045:
URL: https://github.com/apache/ignite/pull/10045#discussion_r883767925
##########
modules/core/src/main/java/org/apache/ignite/internal/util/GridHandleTable.java:
##########
@@ -107,9 +110,15 @@ public int lookup(Object obj) {
* Resets table to its initial (empty) state.
*/
public void clear() {
- System.arraycopy(spineEmpty, 0, spine, 0, spineEmpty.length);
- System.arraycopy(nextEmpty, 0, next, 0, nextEmpty.length);
+ if (size < objs.length) {
+ if (shrink()) {
+ size = 0;
+
+ return;
+ }
+ }
+ Arrays.fill(spine, -1);
Arrays.fill(objs, null);
Review Comment:
You're right, fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] sashapolo commented on a diff in pull request #10045: IGNITE-17043 Shrink GridHandleTable on clear
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #10045:
URL: https://github.com/apache/ignite/pull/10045#discussion_r883549474
##########
modules/core/src/main/java/org/apache/ignite/internal/util/GridHandleTable.java:
##########
@@ -181,6 +203,32 @@ private void growEntries() {
objs = newObjs;
}
+ /**
+ * Tries to gradually shrink hash table by factor of two when it's cleared.
+ *
+ * @return {@code true} if shrinked the table, {@code false} otherwise.
+ */
+ private boolean shrink() {
+ int newLen = objs.length;
+
+ if (newLen > initCap) {
+ int shrinked = (newLen - 1) / 2;
Review Comment:
And why do we do that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] SammyVimes commented on a diff in pull request #10045: IGNITE-17043 Shrink GridHandleTable on clear
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #10045:
URL: https://github.com/apache/ignite/pull/10045#discussion_r883580154
##########
modules/core/src/main/java/org/apache/ignite/internal/util/GridHandleTable.java:
##########
@@ -107,6 +121,14 @@ public int lookup(Object obj) {
* Resets table to its initial (empty) state.
*/
public void clear() {
+ if (size < objs.length) {
+ if (shrink()) {
+ size = 0;
+
+ return;
+ }
+ }
+
System.arraycopy(spineEmpty, 0, spine, 0, spineEmpty.length);
System.arraycopy(nextEmpty, 0, next, 0, nextEmpty.length);
Review Comment:
I like the first suggestion, but the second one will lead to unnecessary complication (because `size` will be affected by this change of indexing) and filling operation shouldn't be very frequent with the fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] macrergate commented on a diff in pull request #10045: IGNITE-17043 Shrink GridHandleTable on clear
Posted by GitBox <gi...@apache.org>.
macrergate commented on code in PR #10045:
URL: https://github.com/apache/ignite/pull/10045#discussion_r883761269
##########
modules/core/src/main/java/org/apache/ignite/internal/util/GridHandleTable.java:
##########
@@ -107,9 +110,15 @@ public int lookup(Object obj) {
* Resets table to its initial (empty) state.
*/
public void clear() {
- System.arraycopy(spineEmpty, 0, spine, 0, spineEmpty.length);
- System.arraycopy(nextEmpty, 0, next, 0, nextEmpty.length);
+ if (size < objs.length) {
+ if (shrink()) {
+ size = 0;
+
+ return;
+ }
+ }
+ Arrays.fill(spine, -1);
Arrays.fill(objs, null);
Review Comment:
Why not to clear only first 'size' elements? The rest must be null anyway.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] SammyVimes commented on a diff in pull request #10045: IGNITE-17043 Shrink GridHandleTable on clear
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #10045:
URL: https://github.com/apache/ignite/pull/10045#discussion_r883773347
##########
modules/core/src/main/java/org/apache/ignite/internal/util/GridHandleTable.java:
##########
@@ -107,6 +121,14 @@ public int lookup(Object obj) {
* Resets table to its initial (empty) state.
*/
public void clear() {
+ if (size < objs.length) {
+ if (shrink()) {
+ size = 0;
+
+ return;
+ }
+ }
+
System.arraycopy(spineEmpty, 0, spine, 0, spineEmpty.length);
System.arraycopy(nextEmpty, 0, next, 0, nextEmpty.length);
Review Comment:
Yes, you're right, I had a little mixup)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] SammyVimes merged pull request #10045: IGNITE-17043 Shrink GridHandleTable on clear
Posted by GitBox <gi...@apache.org>.
SammyVimes merged PR #10045:
URL: https://github.com/apache/ignite/pull/10045
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] macrergate commented on a diff in pull request #10045: IGNITE-17043 Shrink GridHandleTable on clear
Posted by GitBox <gi...@apache.org>.
macrergate commented on code in PR #10045:
URL: https://github.com/apache/ignite/pull/10045#discussion_r883771452
##########
modules/core/src/main/java/org/apache/ignite/internal/util/GridHandleTable.java:
##########
@@ -107,6 +121,14 @@ public int lookup(Object obj) {
* Resets table to its initial (empty) state.
*/
public void clear() {
+ if (size < objs.length) {
+ if (shrink()) {
+ size = 0;
+
+ return;
+ }
+ }
+
System.arraycopy(spineEmpty, 0, spine, 0, spineEmpty.length);
System.arraycopy(nextEmpty, 0, next, 0, nextEmpty.length);
Review Comment:
'Objs' array is filling from [0 to size), all the rest elements are always null.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] macrergate commented on a diff in pull request #10045: IGNITE-17043 Shrink GridHandleTable on clear
Posted by GitBox <gi...@apache.org>.
macrergate commented on code in PR #10045:
URL: https://github.com/apache/ignite/pull/10045#discussion_r883505993
##########
modules/core/src/main/java/org/apache/ignite/internal/util/GridHandleTable.java:
##########
@@ -107,6 +121,14 @@ public int lookup(Object obj) {
* Resets table to its initial (empty) state.
*/
public void clear() {
+ if (size < objs.length) {
+ if (shrink()) {
+ size = 0;
+
+ return;
+ }
+ }
+
System.arraycopy(spineEmpty, 0, spine, 0, spineEmpty.length);
System.arraycopy(nextEmpty, 0, next, 0, nextEmpty.length);
Review Comment:
```suggestion
just some more thoughts:
- I guess that the array 'next' can be not cleaned at all as we are resetting 'size' to zero and we never read the next array elements with index>=size
- we could consider zero value as empty not -1, if we would change postitioning to 'int idx = (hash(obj) % spine.length) + 1; in this case we wouldn't need to fill int arrays in init() method as they have zeroes by default.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] SammyVimes commented on a diff in pull request #10045: IGNITE-17043 Shrink GridHandleTable on clear
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #10045:
URL: https://github.com/apache/ignite/pull/10045#discussion_r883546860
##########
modules/core/src/main/java/org/apache/ignite/internal/util/GridHandleTable.java:
##########
@@ -181,6 +203,32 @@ private void growEntries() {
objs = newObjs;
}
+ /**
+ * Tries to gradually shrink hash table by factor of two when it's cleared.
+ *
+ * @return {@code true} if shrinked the table, {@code false} otherwise.
+ */
+ private boolean shrink() {
+ int newLen = objs.length;
+
+ if (newLen > initCap) {
+ int shrinked = (newLen - 1) / 2;
Review Comment:
Because we grow it as "(size << 1) + 1"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] macrergate commented on a diff in pull request #10045: IGNITE-17043 Shrink GridHandleTable on clear
Posted by GitBox <gi...@apache.org>.
macrergate commented on code in PR #10045:
URL: https://github.com/apache/ignite/pull/10045#discussion_r883597024
##########
modules/core/src/main/java/org/apache/ignite/internal/util/GridHandleTable.java:
##########
@@ -107,6 +121,14 @@ public int lookup(Object obj) {
* Resets table to its initial (empty) state.
*/
public void clear() {
+ if (size < objs.length) {
+ if (shrink()) {
+ size = 0;
+
+ return;
+ }
+ }
+
System.arraycopy(spineEmpty, 0, spine, 0, spineEmpty.length);
System.arraycopy(nextEmpty, 0, next, 0, nextEmpty.length);
Review Comment:
I agree with you, let's ignore the second one, it's arguable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org