<Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

<Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

Krishna Addepalli

Hi All,

 

Please review the fix for bug:

Bug: JDK-8190281 https://bugs.openjdk.java.net/browse/JDK-8190281

JDK 10 Webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev00/

 

This bug was created while root causing JDK-8187936, and the following refactoring points have been addressed:

 

1. Line 927: Uninitialized variables, checking for trivial reject case multiple times. 
2. Line 999: Traditional code written to find maximum size of components, which can be done without any local variables and explicit looping by replacing with streams. 
3. Line 1365: Code repetition for differenct conditions, which can be ored together to reduce the repetition. 
4. Line 1482: A large code block gets repeated only because of different values need to be passed in one line. This can be moved to a variable initialization, and the repeating code blocks can be reduced to one. 
5. Line 1505: Variable initialization can be simplified by combining different conditions. 
6. Line 1540: An explicit loop to apply a function over a collection, can be achieved in one line by a forEach construct.  – This is producing some visual artifacts, so ignored.
7. Line 1747: Combine all the trivial reject cases into one condition, and also, a potential bug which increments the "nextIndex" value beyond the length of the containing elements. The increment should happen only if the trivial reject case fails.

 

Thanks,

Krishna

Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

Krishna Addepalli

Hi Sergey,

 

Per our conversation, I have done the following changes:

1.       Found that the .class size increases by 1kb when streams are used, so reverted the changes related to it.

2.       Moved the “++nextIndex” into the conditional, so that there is no logical change.

Here is the updated webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev01/

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Wednesday, December 6, 2017 2:43 PM
To: [hidden email]
Subject: [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi All,

 

Please review the fix for bug:

Bug: JDK-8190281 https://bugs.openjdk.java.net/browse/JDK-8190281

JDK 10 Webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev00/

 

This bug was created while root causing JDK-8187936, and the following refactoring points have been addressed:

 

1. Line 927: Uninitialized variables, checking for trivial reject case multiple times. 
2. Line 999: Traditional code written to find maximum size of components, which can be done without any local variables and explicit looping by replacing with streams. 
3. Line 1365: Code repetition for differenct conditions, which can be ored together to reduce the repetition. 
4. Line 1482: A large code block gets repeated only because of different values need to be passed in one line. This can be moved to a variable initialization, and the repeating code blocks can be reduced to one. 
5. Line 1505: Variable initialization can be simplified by combining different conditions. 
6. Line 1540: An explicit loop to apply a function over a collection, can be achieved in one line by a forEach construct.  – This is producing some visual artifacts, so ignored.
7. Line 1747: Combine all the trivial reject cases into one condition, and also, a potential bug which increments the "nextIndex" value beyond the length of the containing elements. The increment should happen only if the trivial reject case fails.

 

Thanks,

Krishna

Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

prasanta sadhukhan

Hi Krishna,

This seems good to me except one thing. You are checking getRowCount() == 0 but there is a chance of test extending VariableHeightLayoutCache and overriding getRowCount to return -ve also as it is an int. In that case, I guess this function will not return -1 which spec mandates [If there are no rows, -1 is returned] so I guess we should check for <=0.
Also, there is no need of calling getRowCount() twice as it will not change between 929, 936.

Regards
Prasanta
On 12/7/2017 4:41 PM, Krishna Addepalli wrote:

Hi Sergey,

 

Per our conversation, I have done the following changes:

1.       Found that the .class size increases by 1kb when streams are used, so reverted the changes related to it.

2.       Moved the “++nextIndex” into the conditional, so that there is no logical change.

Here is the updated webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev01/

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Wednesday, December 6, 2017 2:43 PM
To: [hidden email]
Subject: [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi All,

 

Please review the fix for bug:

Bug: JDK-8190281 https://bugs.openjdk.java.net/browse/JDK-8190281

JDK 10 Webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev00/

 

This bug was created while root causing JDK-8187936, and the following refactoring points have been addressed:

 

1. Line 927: Uninitialized variables, checking for trivial reject case multiple times. 
2. Line 999: Traditional code written to find maximum size of components, which can be done without any local variables and explicit looping by replacing with streams. 
3. Line 1365: Code repetition for differenct conditions, which can be ored together to reduce the repetition. 
4. Line 1482: A large code block gets repeated only because of different values need to be passed in one line. This can be moved to a variable initialization, and the repeating code blocks can be reduced to one. 
5. Line 1505: Variable initialization can be simplified by combining different conditions. 
6. Line 1540: An explicit loop to apply a function over a collection, can be achieved in one line by a forEach construct.  – This is producing some visual artifacts, so ignored.
7. Line 1747: Combine all the trivial reject cases into one condition, and also, a potential bug which increments the "nextIndex" value beyond the length of the containing elements. The increment should happen only if the trivial reject case fails.

 

Thanks,

Krishna


Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

Krishna Addepalli

Hi Prasanta,

 

Thanks for pointing out the “getRowCount()==0” check. Modified it to “getRowCount() <= 0” in the new webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev02/

 

As for calling the method twice, you are right that we don’t need to call it twice, but in the interest of having trivial reject case first, and then start the variable declarations, had to let be there to be called twice. Precisely for the reason you stated, it shouldn’t matter if we called it twice.

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Saturday, December 9, 2017 7:54 PM
To: Krishna Addepalli <[hidden email]>; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi Krishna,

This seems good to me except one thing. You are checking getRowCount() == 0 but there is a chance of test extending VariableHeightLayoutCache and overriding getRowCount to return -ve also as it is an int. In that case, I guess this function will not return -1 which spec mandates [If there are no rows, -1 is returned] so I guess we should check for <=0.
Also, there is no need of calling getRowCount() twice as it will not change between 929, 936.

Regards
Prasanta

On 12/7/2017 4:41 PM, Krishna Addepalli wrote:

Hi Sergey,

 

Per our conversation, I have done the following changes:

1.       Found that the .class size increases by 1kb when streams are used, so reverted the changes related to it.

2.       Moved the “++nextIndex” into the conditional, so that there is no logical change.

Here is the updated webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev01/

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Wednesday, December 6, 2017 2:43 PM
To: [hidden email]
Subject: [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi All,

 

Please review the fix for bug:

Bug: JDK-8190281 https://bugs.openjdk.java.net/browse/JDK-8190281

JDK 10 Webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev00/

 

This bug was created while root causing JDK-8187936, and the following refactoring points have been addressed:

 

1. Line 927: Uninitialized variables, checking for trivial reject case multiple times. 
2. Line 999: Traditional code written to find maximum size of components, which can be done without any local variables and explicit looping by replacing with streams. 
3. Line 1365: Code repetition for differenct conditions, which can be ored together to reduce the repetition. 
4. Line 1482: A large code block gets repeated only because of different values need to be passed in one line. This can be moved to a variable initialization, and the repeating code blocks can be reduced to one. 
5. Line 1505: Variable initialization can be simplified by combining different conditions. 
6. Line 1540: An explicit loop to apply a function over a collection, can be achieved in one line by a forEach construct.  – This is producing some visual artifacts, so ignored.
7. Line 1747: Combine all the trivial reject cases into one condition, and also, a potential bug which increments the "nextIndex" value beyond the length of the containing elements. The increment should happen only if the trivial reject case fails.

 

Thanks,

Krishna

 

Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

prasanta sadhukhan

Hi Krishna,

My point was we can call getRowCount() once at first and store the result and use it subsequently. There was no need to call it 2-3 times.

Regards
Prasanta
On 12/11/2017 3:01 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Thanks for pointing out the “getRowCount()==0” check. Modified it to “getRowCount() <= 0” in the new webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev02/

 

As for calling the method twice, you are right that we don’t need to call it twice, but in the interest of having trivial reject case first, and then start the variable declarations, had to let be there to be called twice. Precisely for the reason you stated, it shouldn’t matter if we called it twice.

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Saturday, December 9, 2017 7:54 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi Krishna,

This seems good to me except one thing. You are checking getRowCount() == 0 but there is a chance of test extending VariableHeightLayoutCache and overriding getRowCount to return -ve also as it is an int. In that case, I guess this function will not return -1 which spec mandates [If there are no rows, -1 is returned] so I guess we should check for <=0.
Also, there is no need of calling getRowCount() twice as it will not change between 929, 936.

Regards
Prasanta

On 12/7/2017 4:41 PM, Krishna Addepalli wrote:

Hi Sergey,

 

Per our conversation, I have done the following changes:

1.       Found that the .class size increases by 1kb when streams are used, so reverted the changes related to it.

2.       Moved the “++nextIndex” into the conditional, so that there is no logical change.

Here is the updated webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev01/

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Wednesday, December 6, 2017 2:43 PM
To: [hidden email]
Subject: [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi All,

 

Please review the fix for bug:

Bug: JDK-8190281 https://bugs.openjdk.java.net/browse/JDK-8190281

JDK 10 Webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev00/

 

This bug was created while root causing JDK-8187936, and the following refactoring points have been addressed:

 

1. Line 927: Uninitialized variables, checking for trivial reject case multiple times. 
2. Line 999: Traditional code written to find maximum size of components, which can be done without any local variables and explicit looping by replacing with streams. 
3. Line 1365: Code repetition for differenct conditions, which can be ored together to reduce the repetition. 
4. Line 1482: A large code block gets repeated only because of different values need to be passed in one line. This can be moved to a variable initialization, and the repeating code blocks can be reduced to one. 
5. Line 1505: Variable initialization can be simplified by combining different conditions. 
6. Line 1540: An explicit loop to apply a function over a collection, can be achieved in one line by a forEach construct.  – This is producing some visual artifacts, so ignored.
7. Line 1747: Combine all the trivial reject cases into one condition, and also, a potential bug which increments the "nextIndex" value beyond the length of the containing elements. The increment should happen only if the trivial reject case fails.

 

Thanks,

Krishna

 


Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

Krishna Addepalli

Hi Prasanta,

 

Yes, you are right, but as I mentioned earlier, that would need to make one variable declaration for caching before trivial reject case, which I wanted to avoid.

As for the body of getRowCount() it is just returning “visibleNodes.size()”, which shouldn’t be a (performance)problem if called 2 times as I understand.

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Monday, December 11, 2017 4:02 PM
To: Krishna Addepalli <[hidden email]>; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi Krishna,

My point was we can call getRowCount() once at first and store the result and use it subsequently. There was no need to call it 2-3 times.

Regards
Prasanta

On 12/11/2017 3:01 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Thanks for pointing out the “getRowCount()==0” check. Modified it to “getRowCount() <= 0” in the new webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev02/

 

As for calling the method twice, you are right that we don’t need to call it twice, but in the interest of having trivial reject case first, and then start the variable declarations, had to let be there to be called twice. Precisely for the reason you stated, it shouldn’t matter if we called it twice.

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Saturday, December 9, 2017 7:54 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi Krishna,

This seems good to me except one thing. You are checking getRowCount() == 0 but there is a chance of test extending VariableHeightLayoutCache and overriding getRowCount to return -ve also as it is an int. In that case, I guess this function will not return -1 which spec mandates [If there are no rows, -1 is returned] so I guess we should check for <=0.
Also, there is no need of calling getRowCount() twice as it will not change between 929, 936.

Regards
Prasanta

On 12/7/2017 4:41 PM, Krishna Addepalli wrote:

Hi Sergey,

 

Per our conversation, I have done the following changes:

1.       Found that the .class size increases by 1kb when streams are used, so reverted the changes related to it.

2.       Moved the “++nextIndex” into the conditional, so that there is no logical change.

Here is the updated webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev01/

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Wednesday, December 6, 2017 2:43 PM
To: [hidden email]
Subject: [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi All,

 

Please review the fix for bug:

Bug: JDK-8190281 https://bugs.openjdk.java.net/browse/JDK-8190281

JDK 10 Webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev00/

 

This bug was created while root causing JDK-8187936, and the following refactoring points have been addressed:

 

1. Line 927: Uninitialized variables, checking for trivial reject case multiple times. 
2. Line 999: Traditional code written to find maximum size of components, which can be done without any local variables and explicit looping by replacing with streams. 
3. Line 1365: Code repetition for differenct conditions, which can be ored together to reduce the repetition. 
4. Line 1482: A large code block gets repeated only because of different values need to be passed in one line. This can be moved to a variable initialization, and the repeating code blocks can be reduced to one. 
5. Line 1505: Variable initialization can be simplified by combining different conditions. 
6. Line 1540: An explicit loop to apply a function over a collection, can be achieved in one line by a forEach construct.  – This is producing some visual artifacts, so ignored.
7. Line 1747: Combine all the trivial reject cases into one condition, and also, a potential bug which increments the "nextIndex" value beyond the length of the containing elements. The increment should happen only if the trivial reject case fails.

 

Thanks,

Krishna

 

 

Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

prasanta sadhukhan



On 12/11/2017 4:16 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Yes, you are right, but as I mentioned earlier, that would need to make one variable declaration for caching before trivial reject case, which I wanted to avoid.

As for the body of getRowCount() it is just returning “visibleNodes.size()”, which shouldn’t be a (performance)problem if called 2 times as I understand.

But, the whole premise of changing getRowCount() <=0  was that it can be overridden and return -ve. Left to present implementation, we would not have needed "less than" check.
So, if we are changing one case because of the above reason, then we cannot forego the 2nd case's problem, as it can have any implementation.

Regards
Prasanta

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Monday, December 11, 2017 4:02 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi Krishna,

My point was we can call getRowCount() once at first and store the result and use it subsequently. There was no need to call it 2-3 times.

Regards
Prasanta

On 12/11/2017 3:01 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Thanks for pointing out the “getRowCount()==0” check. Modified it to “getRowCount() <= 0” in the new webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev02/

 

As for calling the method twice, you are right that we don’t need to call it twice, but in the interest of having trivial reject case first, and then start the variable declarations, had to let be there to be called twice. Precisely for the reason you stated, it shouldn’t matter if we called it twice.

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Saturday, December 9, 2017 7:54 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi Krishna,

This seems good to me except one thing. You are checking getRowCount() == 0 but there is a chance of test extending VariableHeightLayoutCache and overriding getRowCount to return -ve also as it is an int. In that case, I guess this function will not return -1 which spec mandates [If there are no rows, -1 is returned] so I guess we should check for <=0.
Also, there is no need of calling getRowCount() twice as it will not change between 929, 936.

Regards
Prasanta

On 12/7/2017 4:41 PM, Krishna Addepalli wrote:

Hi Sergey,

 

Per our conversation, I have done the following changes:

1.       Found that the .class size increases by 1kb when streams are used, so reverted the changes related to it.

2.       Moved the “++nextIndex” into the conditional, so that there is no logical change.

Here is the updated webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev01/

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Wednesday, December 6, 2017 2:43 PM
To: [hidden email]
Subject: [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi All,

 

Please review the fix for bug:

Bug: JDK-8190281 https://bugs.openjdk.java.net/browse/JDK-8190281

JDK 10 Webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev00/

 

This bug was created while root causing JDK-8187936, and the following refactoring points have been addressed:

 

1. Line 927: Uninitialized variables, checking for trivial reject case multiple times. 
2. Line 999: Traditional code written to find maximum size of components, which can be done without any local variables and explicit looping by replacing with streams. 
3. Line 1365: Code repetition for differenct conditions, which can be ored together to reduce the repetition. 
4. Line 1482: A large code block gets repeated only because of different values need to be passed in one line. This can be moved to a variable initialization, and the repeating code blocks can be reduced to one. 
5. Line 1505: Variable initialization can be simplified by combining different conditions. 
6. Line 1540: An explicit loop to apply a function over a collection, can be achieved in one line by a forEach construct.  – This is producing some visual artifacts, so ignored.
7. Line 1747: Combine all the trivial reject cases into one condition, and also, a potential bug which increments the "nextIndex" value beyond the length of the containing elements. The increment should happen only if the trivial reject case fails.

 

Thanks,

Krishna

 

 


Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

Krishna Addepalli

Hi Prasanta,

 

Did the change for caching the result of calling “getRowCount()” into a variable, as pointed out by you, and here is the new webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev03/

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Monday, December 11, 2017 7:24 PM
To: Krishna Addepalli <[hidden email]>; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

 

 

On 12/11/2017 4:16 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Yes, you are right, but as I mentioned earlier, that would need to make one variable declaration for caching before trivial reject case, which I wanted to avoid.

As for the body of getRowCount() it is just returning “visibleNodes.size()”, which shouldn’t be a (performance)problem if called 2 times as I understand.

But, the whole premise of changing getRowCount() <=0  was that it can be overridden and return -ve. Left to present implementation, we would not have needed "less than" check.
So, if we are changing one case because of the above reason, then we cannot forego the 2nd case's problem, as it can have any implementation.

Regards
Prasanta

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Monday, December 11, 2017 4:02 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi Krishna,

My point was we can call getRowCount() once at first and store the result and use it subsequently. There was no need to call it 2-3 times.

Regards
Prasanta

On 12/11/2017 3:01 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Thanks for pointing out the “getRowCount()==0” check. Modified it to “getRowCount() <= 0” in the new webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev02/

 

As for calling the method twice, you are right that we don’t need to call it twice, but in the interest of having trivial reject case first, and then start the variable declarations, had to let be there to be called twice. Precisely for the reason you stated, it shouldn’t matter if we called it twice.

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Saturday, December 9, 2017 7:54 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi Krishna,

This seems good to me except one thing. You are checking getRowCount() == 0 but there is a chance of test extending VariableHeightLayoutCache and overriding getRowCount to return -ve also as it is an int. In that case, I guess this function will not return -1 which spec mandates [If there are no rows, -1 is returned] so I guess we should check for <=0.
Also, there is no need of calling getRowCount() twice as it will not change between 929, 936.

Regards
Prasanta

On 12/7/2017 4:41 PM, Krishna Addepalli wrote:

Hi Sergey,

 

Per our conversation, I have done the following changes:

1.       Found that the .class size increases by 1kb when streams are used, so reverted the changes related to it.

2.       Moved the “++nextIndex” into the conditional, so that there is no logical change.

Here is the updated webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev01/

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Wednesday, December 6, 2017 2:43 PM
To: [hidden email]
Subject: [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi All,

 

Please review the fix for bug:

Bug: JDK-8190281 https://bugs.openjdk.java.net/browse/JDK-8190281

JDK 10 Webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev00/

 

This bug was created while root causing JDK-8187936, and the following refactoring points have been addressed:

 

1. Line 927: Uninitialized variables, checking for trivial reject case multiple times. 
2. Line 999: Traditional code written to find maximum size of components, which can be done without any local variables and explicit looping by replacing with streams. 
3. Line 1365: Code repetition for differenct conditions, which can be ored together to reduce the repetition. 
4. Line 1482: A large code block gets repeated only because of different values need to be passed in one line. This can be moved to a variable initialization, and the repeating code blocks can be reduced to one. 
5. Line 1505: Variable initialization can be simplified by combining different conditions. 
6. Line 1540: An explicit loop to apply a function over a collection, can be achieved in one line by a forEach construct.  – This is producing some visual artifacts, so ignored.
7. Line 1747: Combine all the trivial reject cases into one condition, and also, a potential bug which increments the "nextIndex" value beyond the length of the containing elements. The increment should happen only if the trivial reject case fails.

 

Thanks,

Krishna

 

 

 

Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

prasanta sadhukhan

You missed using the variable at l933

Regards
Prasanta
On 12/12/2017 5:21 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Did the change for caching the result of calling “getRowCount()” into a variable, as pointed out by you, and here is the new webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev03/

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Monday, December 11, 2017 7:24 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

 

 

On 12/11/2017 4:16 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Yes, you are right, but as I mentioned earlier, that would need to make one variable declaration for caching before trivial reject case, which I wanted to avoid.

As for the body of getRowCount() it is just returning “visibleNodes.size()”, which shouldn’t be a (performance)problem if called 2 times as I understand.

But, the whole premise of changing getRowCount() <=0  was that it can be overridden and return -ve. Left to present implementation, we would not have needed "less than" check.
So, if we are changing one case because of the above reason, then we cannot forego the 2nd case's problem, as it can have any implementation.

Regards
Prasanta

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Monday, December 11, 2017 4:02 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi Krishna,

My point was we can call getRowCount() once at first and store the result and use it subsequently. There was no need to call it 2-3 times.

Regards
Prasanta

On 12/11/2017 3:01 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Thanks for pointing out the “getRowCount()==0” check. Modified it to “getRowCount() <= 0” in the new webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev02/

 

As for calling the method twice, you are right that we don’t need to call it twice, but in the interest of having trivial reject case first, and then start the variable declarations, had to let be there to be called twice. Precisely for the reason you stated, it shouldn’t matter if we called it twice.

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Saturday, December 9, 2017 7:54 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi Krishna,

This seems good to me except one thing. You are checking getRowCount() == 0 but there is a chance of test extending VariableHeightLayoutCache and overriding getRowCount to return -ve also as it is an int. In that case, I guess this function will not return -1 which spec mandates [If there are no rows, -1 is returned] so I guess we should check for <=0.
Also, there is no need of calling getRowCount() twice as it will not change between 929, 936.

Regards
Prasanta

On 12/7/2017 4:41 PM, Krishna Addepalli wrote:

Hi Sergey,

 

Per our conversation, I have done the following changes:

1.       Found that the .class size increases by 1kb when streams are used, so reverted the changes related to it.

2.       Moved the “++nextIndex” into the conditional, so that there is no logical change.

Here is the updated webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev01/

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Wednesday, December 6, 2017 2:43 PM
To: [hidden email]
Subject: [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi All,

 

Please review the fix for bug:

Bug: JDK-8190281 https://bugs.openjdk.java.net/browse/JDK-8190281

JDK 10 Webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev00/

 

This bug was created while root causing JDK-8187936, and the following refactoring points have been addressed:

 

1. Line 927: Uninitialized variables, checking for trivial reject case multiple times. 
2. Line 999: Traditional code written to find maximum size of components, which can be done without any local variables and explicit looping by replacing with streams. 
3. Line 1365: Code repetition for differenct conditions, which can be ored together to reduce the repetition. 
4. Line 1482: A large code block gets repeated only because of different values need to be passed in one line. This can be moved to a variable initialization, and the repeating code blocks can be reduced to one. 
5. Line 1505: Variable initialization can be simplified by combining different conditions. 
6. Line 1540: An explicit loop to apply a function over a collection, can be achieved in one line by a forEach construct.  – This is producing some visual artifacts, so ignored.
7. Line 1747: Combine all the trivial reject cases into one condition, and also, a potential bug which increments the "nextIndex" value beyond the length of the containing elements. The increment should happen only if the trivial reject case fails.

 

Thanks,

Krishna

 

 

 


Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

prasanta sadhukhan



On 12/12/2017 7:04 PM, Prasanta Sadhukhan wrote:

You missed using the variable at l933

and l955,956

Regards
Prasanta
On 12/12/2017 5:21 PM, Krishna Addepalli wrote:

Hi Prasanta,

Did the change for caching the result of calling �getRowCount()� into a variable, as pointed out by you, and here is the new webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev03/

Thanks,

Krishna

From: Prasanta Sadhukhan
Sent: Monday, December 11, 2017 7:24 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

On 12/11/2017 4:16 PM, Krishna Addepalli wrote:

Hi Prasanta,

Yes, you are right, but as I mentioned earlier, that would need to make one variable declaration for caching before trivial reject case, which I wanted to avoid.

As for the body of getRowCount() it is just returning �visibleNodes.size()�, which shouldn�t be a (performance)problem if called 2 times as I understand.

But, the whole premise of changing getRowCount() <=0� was that it can be overridden and return -ve. Left to present implementation, we would not have needed "less than" check.
So, if we are changing one case because of the above reason, then we cannot forego the 2nd case's problem, as it can have any implementation.

Regards
Prasanta

Thanks,

Krishna

From: Prasanta Sadhukhan
Sent: Monday, December 11, 2017 4:02 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

Hi Krishna,

My point was we can call getRowCount() once at first and store the result and use it subsequently. There was no need to call it 2-3 times.

Regards
Prasanta

On 12/11/2017 3:01 PM, Krishna Addepalli wrote:

Hi Prasanta,

Thanks for pointing out the �getRowCount()==0� check. Modified it to �getRowCount() <= 0� in the new webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev02/

As for calling the method twice, you are right that we don�t need to call it twice, but in the interest of having trivial reject case first, and then start the variable declarations, had to let be there to be called twice. Precisely for the reason you stated, it shouldn�t matter if we called it twice.

Thanks,

Krishna

From: Prasanta Sadhukhan
Sent: Saturday, December 9, 2017 7:54 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

Hi Krishna,

This seems good to me except one thing. You are checking getRowCount() == 0 but there is a chance of test extending VariableHeightLayoutCache and overriding getRowCount to return -ve also as it is an int. In that case, I guess this function will not return -1 which spec mandates [If there are no rows, -1 is returned] so I guess we should check for <=0.
Also, there is no need of calling getRowCount() twice as it will not change between 929, 936.

Regards
Prasanta

On 12/7/2017 4:41 PM, Krishna Addepalli wrote:

Hi Sergey,

Per our conversation, I have done the following changes:

1.������ Found that the .class size increases by 1kb when streams are used, so reverted the changes related to it.

2.������ Moved the �++nextIndex� into the conditional, so that there is no logical change.

Here is the updated webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev01/

Thanks,

Krishna

From: Krishna Addepalli
Sent: Wednesday, December 6, 2017 2:43 PM
To: [hidden email]
Subject: [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

Hi All,

Please review the fix for bug:

Bug: JDK-8190281 https://bugs.openjdk.java.net/browse/JDK-8190281

JDK 10 Webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev00/

This bug was created while root causing JDK-8187936, and the following refactoring points have been addressed:

1. Line 927: Uninitialized variables, checking for trivial reject case multiple times.�
2. Line 999: Traditional code written to find maximum size of components, which can be done without any local variables and explicit looping by replacing with streams.�
3. Line 1365: Code repetition for differenct conditions, which can be ored together to reduce the repetition.�
4. Line 1482: A large code block gets repeated only because of different values need to be passed in one line. This can be moved to a variable initialization, and the repeating code blocks can be reduced to one.�
5. Line 1505: Variable initialization can be simplified by combining different conditions.�
6. Line 1540: An explicit loop to apply a function over a collection, can be achieved in one line by a forEach construct.� � This is producing some visual artifacts, so ignored.
7. Line 1747: Combine all the trivial reject cases into one condition, and also, a potential bug which increments the "nextIndex" value beyond the length of the containing elements. The increment should happen only if the trivial reject case fails.

Thanks,

Krishna



Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

Krishna Addepalli
In reply to this post by prasanta sadhukhan

Oops! My bad. Created a new webrev here with the correction: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev04/

 

From: Prasanta Sadhukhan
Sent: Tuesday, December 12, 2017 7:05 PM
To: Krishna Addepalli <[hidden email]>; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

You missed using the variable at l933

Regards
Prasanta

On 12/12/2017 5:21 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Did the change for caching the result of calling “getRowCount()” into a variable, as pointed out by you, and here is the new webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev03/

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Monday, December 11, 2017 7:24 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

 

 

On 12/11/2017 4:16 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Yes, you are right, but as I mentioned earlier, that would need to make one variable declaration for caching before trivial reject case, which I wanted to avoid.

As for the body of getRowCount() it is just returning “visibleNodes.size()”, which shouldn’t be a (performance)problem if called 2 times as I understand.

But, the whole premise of changing getRowCount() <=0  was that it can be overridden and return -ve. Left to present implementation, we would not have needed "less than" check.
So, if we are changing one case because of the above reason, then we cannot forego the 2nd case's problem, as it can have any implementation.

Regards
Prasanta


 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Monday, December 11, 2017 4:02 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi Krishna,

My point was we can call getRowCount() once at first and store the result and use it subsequently. There was no need to call it 2-3 times.

Regards
Prasanta

On 12/11/2017 3:01 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Thanks for pointing out the “getRowCount()==0” check. Modified it to “getRowCount() <= 0” in the new webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev02/

 

As for calling the method twice, you are right that we don’t need to call it twice, but in the interest of having trivial reject case first, and then start the variable declarations, had to let be there to be called twice. Precisely for the reason you stated, it shouldn’t matter if we called it twice.

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Saturday, December 9, 2017 7:54 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi Krishna,

This seems good to me except one thing. You are checking getRowCount() == 0 but there is a chance of test extending VariableHeightLayoutCache and overriding getRowCount to return -ve also as it is an int. In that case, I guess this function will not return -1 which spec mandates [If there are no rows, -1 is returned] so I guess we should check for <=0.
Also, there is no need of calling getRowCount() twice as it will not change between 929, 936.

Regards
Prasanta

On 12/7/2017 4:41 PM, Krishna Addepalli wrote:

Hi Sergey,

 

Per our conversation, I have done the following changes:

1.       Found that the .class size increases by 1kb when streams are used, so reverted the changes related to it.

2.       Moved the “++nextIndex” into the conditional, so that there is no logical change.

Here is the updated webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev01/

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Wednesday, December 6, 2017 2:43 PM
To: [hidden email]
Subject: [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi All,

 

Please review the fix for bug:

Bug: JDK-8190281 https://bugs.openjdk.java.net/browse/JDK-8190281

JDK 10 Webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev00/

 

This bug was created while root causing JDK-8187936, and the following refactoring points have been addressed:

 

1. Line 927: Uninitialized variables, checking for trivial reject case multiple times. 
2. Line 999: Traditional code written to find maximum size of components, which can be done without any local variables and explicit looping by replacing with streams. 
3. Line 1365: Code repetition for differenct conditions, which can be ored together to reduce the repetition. 
4. Line 1482: A large code block gets repeated only because of different values need to be passed in one line. This can be moved to a variable initialization, and the repeating code blocks can be reduced to one. 
5. Line 1505: Variable initialization can be simplified by combining different conditions. 
6. Line 1540: An explicit loop to apply a function over a collection, can be achieved in one line by a forEach construct.  – This is producing some visual artifacts, so ignored.
7. Line 1747: Combine all the trivial reject cases into one condition, and also, a potential bug which increments the "nextIndex" value beyond the length of the containing elements. The increment should happen only if the trivial reject case fails.

 

Thanks,

Krishna

 

 

 

 

Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

prasanta sadhukhan

As told, you overlooked l955,956

Regards
Prasanta
On 12/12/2017 7:37 PM, Krishna Addepalli wrote:

Oops! My bad. Created a new webrev here with the correction: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev04/

 

From: Prasanta Sadhukhan
Sent: Tuesday, December 12, 2017 7:05 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

You missed using the variable at l933

Regards
Prasanta

On 12/12/2017 5:21 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Did the change for caching the result of calling “getRowCount()” into a variable, as pointed out by you, and here is the new webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev03/

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Monday, December 11, 2017 7:24 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

 

 

On 12/11/2017 4:16 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Yes, you are right, but as I mentioned earlier, that would need to make one variable declaration for caching before trivial reject case, which I wanted to avoid.

As for the body of getRowCount() it is just returning “visibleNodes.size()”, which shouldn’t be a (performance)problem if called 2 times as I understand.

But, the whole premise of changing getRowCount() <=0  was that it can be overridden and return -ve. Left to present implementation, we would not have needed "less than" check.
So, if we are changing one case because of the above reason, then we cannot forego the 2nd case's problem, as it can have any implementation.

Regards
Prasanta


 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Monday, December 11, 2017 4:02 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi Krishna,

My point was we can call getRowCount() once at first and store the result and use it subsequently. There was no need to call it 2-3 times.

Regards
Prasanta

On 12/11/2017 3:01 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Thanks for pointing out the “getRowCount()==0” check. Modified it to “getRowCount() <= 0” in the new webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev02/

 

As for calling the method twice, you are right that we don’t need to call it twice, but in the interest of having trivial reject case first, and then start the variable declarations, had to let be there to be called twice. Precisely for the reason you stated, it shouldn’t matter if we called it twice.

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Saturday, December 9, 2017 7:54 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi Krishna,

This seems good to me except one thing. You are checking getRowCount() == 0 but there is a chance of test extending VariableHeightLayoutCache and overriding getRowCount to return -ve also as it is an int. In that case, I guess this function will not return -1 which spec mandates [If there are no rows, -1 is returned] so I guess we should check for <=0.
Also, there is no need of calling getRowCount() twice as it will not change between 929, 936.

Regards
Prasanta

On 12/7/2017 4:41 PM, Krishna Addepalli wrote:

Hi Sergey,

 

Per our conversation, I have done the following changes:

1.       Found that the .class size increases by 1kb when streams are used, so reverted the changes related to it.

2.       Moved the “++nextIndex” into the conditional, so that there is no logical change.

Here is the updated webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev01/

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Wednesday, December 6, 2017 2:43 PM
To: [hidden email]
Subject: [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi All,

 

Please review the fix for bug:

Bug: JDK-8190281 https://bugs.openjdk.java.net/browse/JDK-8190281

JDK 10 Webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev00/

 

This bug was created while root causing JDK-8187936, and the following refactoring points have been addressed:

 

1. Line 927: Uninitialized variables, checking for trivial reject case multiple times. 
2. Line 999: Traditional code written to find maximum size of components, which can be done without any local variables and explicit looping by replacing with streams. 
3. Line 1365: Code repetition for differenct conditions, which can be ored together to reduce the repetition. 
4. Line 1482: A large code block gets repeated only because of different values need to be passed in one line. This can be moved to a variable initialization, and the repeating code blocks can be reduced to one. 
5. Line 1505: Variable initialization can be simplified by combining different conditions. 
6. Line 1540: An explicit loop to apply a function over a collection, can be achieved in one line by a forEach construct.  – This is producing some visual artifacts, so ignored.
7. Line 1747: Combine all the trivial reject cases into one condition, and also, a potential bug which increments the "nextIndex" value beyond the length of the containing elements. The increment should happen only if the trivial reject case fails.

 

Thanks,

Krishna

 

 

 

 


Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

Krishna Addepalli

Hi Prasanta,

 

The getRowCount() calls l955,956 cannot be removed, since max variable is getting modified in the while loop at l945. There is no guarantee that max will still hold the getRowCount() after the loop exits. So, those calls cannot be removed.

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Tuesday, December 12, 2017 8:08 PM
To: Krishna Addepalli <[hidden email]>; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

As told, you overlooked l955,956

Regards
Prasanta

On 12/12/2017 7:37 PM, Krishna Addepalli wrote:

Oops! My bad. Created a new webrev here with the correction: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev04/

 

From: Prasanta Sadhukhan
Sent: Tuesday, December 12, 2017 7:05 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

You missed using the variable at l933

Regards
Prasanta

On 12/12/2017 5:21 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Did the change for caching the result of calling “getRowCount()” into a variable, as pointed out by you, and here is the new webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev03/

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Monday, December 11, 2017 7:24 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

 

 

On 12/11/2017 4:16 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Yes, you are right, but as I mentioned earlier, that would need to make one variable declaration for caching before trivial reject case, which I wanted to avoid.

As for the body of getRowCount() it is just returning “visibleNodes.size()”, which shouldn’t be a (performance)problem if called 2 times as I understand.

But, the whole premise of changing getRowCount() <=0  was that it can be overridden and return -ve. Left to present implementation, we would not have needed "less than" check.
So, if we are changing one case because of the above reason, then we cannot forego the 2nd case's problem, as it can have any implementation.

Regards
Prasanta



 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Monday, December 11, 2017 4:02 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi Krishna,

My point was we can call getRowCount() once at first and store the result and use it subsequently. There was no need to call it 2-3 times.

Regards
Prasanta

On 12/11/2017 3:01 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Thanks for pointing out the “getRowCount()==0” check. Modified it to “getRowCount() <= 0” in the new webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev02/

 

As for calling the method twice, you are right that we don’t need to call it twice, but in the interest of having trivial reject case first, and then start the variable declarations, had to let be there to be called twice. Precisely for the reason you stated, it shouldn’t matter if we called it twice.

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Saturday, December 9, 2017 7:54 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi Krishna,

This seems good to me except one thing. You are checking getRowCount() == 0 but there is a chance of test extending VariableHeightLayoutCache and overriding getRowCount to return -ve also as it is an int. In that case, I guess this function will not return -1 which spec mandates [If there are no rows, -1 is returned] so I guess we should check for <=0.
Also, there is no need of calling getRowCount() twice as it will not change between 929, 936.

Regards
Prasanta

On 12/7/2017 4:41 PM, Krishna Addepalli wrote:

Hi Sergey,

 

Per our conversation, I have done the following changes:

1.       Found that the .class size increases by 1kb when streams are used, so reverted the changes related to it.

2.       Moved the “++nextIndex” into the conditional, so that there is no logical change.

Here is the updated webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev01/

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Wednesday, December 6, 2017 2:43 PM
To: [hidden email]
Subject: [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi All,

 

Please review the fix for bug:

Bug: JDK-8190281 https://bugs.openjdk.java.net/browse/JDK-8190281

JDK 10 Webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev00/

 

This bug was created while root causing JDK-8187936, and the following refactoring points have been addressed:

 

1. Line 927: Uninitialized variables, checking for trivial reject case multiple times. 
2. Line 999: Traditional code written to find maximum size of components, which can be done without any local variables and explicit looping by replacing with streams. 
3. Line 1365: Code repetition for differenct conditions, which can be ored together to reduce the repetition. 
4. Line 1482: A large code block gets repeated only because of different values need to be passed in one line. This can be moved to a variable initialization, and the repeating code blocks can be reduced to one. 
5. Line 1505: Variable initialization can be simplified by combining different conditions. 
6. Line 1540: An explicit loop to apply a function over a collection, can be achieved in one line by a forEach construct.  – This is producing some visual artifacts, so ignored.
7. Line 1747: Combine all the trivial reject cases into one condition, and also, a potential bug which increments the "nextIndex" value beyond the length of the containing elements. The increment should happen only if the trivial reject case fails.

 

Thanks,

Krishna

 

 

 

 

 

Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

prasanta sadhukhan

But there is no compulsion that we need to store getRowCount() in "max". You can store in some other variable and then "max" point to that in the loop.

Regards
Prasanta
On 12/12/2017 9:51 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

The getRowCount() calls l955,956 cannot be removed, since max variable is getting modified in the while loop at l945. There is no guarantee that max will still hold the getRowCount() after the loop exits. So, those calls cannot be removed.

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Tuesday, December 12, 2017 8:08 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

As told, you overlooked l955,956

Regards
Prasanta

On 12/12/2017 7:37 PM, Krishna Addepalli wrote:

Oops! My bad. Created a new webrev here with the correction: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev04/

 

From: Prasanta Sadhukhan
Sent: Tuesday, December 12, 2017 7:05 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

You missed using the variable at l933

Regards
Prasanta

On 12/12/2017 5:21 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Did the change for caching the result of calling “getRowCount()” into a variable, as pointed out by you, and here is the new webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev03/

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Monday, December 11, 2017 7:24 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

 

 

On 12/11/2017 4:16 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Yes, you are right, but as I mentioned earlier, that would need to make one variable declaration for caching before trivial reject case, which I wanted to avoid.

As for the body of getRowCount() it is just returning “visibleNodes.size()”, which shouldn’t be a (performance)problem if called 2 times as I understand.

But, the whole premise of changing getRowCount() <=0  was that it can be overridden and return -ve. Left to present implementation, we would not have needed "less than" check.
So, if we are changing one case because of the above reason, then we cannot forego the 2nd case's problem, as it can have any implementation.

Regards
Prasanta



 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Monday, December 11, 2017 4:02 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi Krishna,

My point was we can call getRowCount() once at first and store the result and use it subsequently. There was no need to call it 2-3 times.

Regards
Prasanta

On 12/11/2017 3:01 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Thanks for pointing out the “getRowCount()==0” check. Modified it to “getRowCount() <= 0” in the new webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev02/

 

As for calling the method twice, you are right that we don’t need to call it twice, but in the interest of having trivial reject case first, and then start the variable declarations, had to let be there to be called twice. Precisely for the reason you stated, it shouldn’t matter if we called it twice.

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Saturday, December 9, 2017 7:54 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi Krishna,

This seems good to me except one thing. You are checking getRowCount() == 0 but there is a chance of test extending VariableHeightLayoutCache and overriding getRowCount to return -ve also as it is an int. In that case, I guess this function will not return -1 which spec mandates [If there are no rows, -1 is returned] so I guess we should check for <=0.
Also, there is no need of calling getRowCount() twice as it will not change between 929, 936.

Regards
Prasanta

On 12/7/2017 4:41 PM, Krishna Addepalli wrote:

Hi Sergey,

 

Per our conversation, I have done the following changes:

1.       Found that the .class size increases by 1kb when streams are used, so reverted the changes related to it.

2.       Moved the “++nextIndex” into the conditional, so that there is no logical change.

Here is the updated webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev01/

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Wednesday, December 6, 2017 2:43 PM
To: [hidden email]
Subject: [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi All,

 

Please review the fix for bug:

Bug: JDK-8190281 https://bugs.openjdk.java.net/browse/JDK-8190281

JDK 10 Webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev00/

 

This bug was created while root causing JDK-8187936, and the following refactoring points have been addressed:

 

1. Line 927: Uninitialized variables, checking for trivial reject case multiple times. 
2. Line 999: Traditional code written to find maximum size of components, which can be done without any local variables and explicit looping by replacing with streams. 
3. Line 1365: Code repetition for differenct conditions, which can be ored together to reduce the repetition. 
4. Line 1482: A large code block gets repeated only because of different values need to be passed in one line. This can be moved to a variable initialization, and the repeating code blocks can be reduced to one. 
5. Line 1505: Variable initialization can be simplified by combining different conditions. 
6. Line 1540: An explicit loop to apply a function over a collection, can be achieved in one line by a forEach construct.  – This is producing some visual artifacts, so ignored.
7. Line 1747: Combine all the trivial reject cases into one condition, and also, a potential bug which increments the "nextIndex" value beyond the length of the containing elements. The increment should happen only if the trivial reject case fails.

 

Thanks,

Krishna

 

 

 

 

 


Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

Krishna Addepalli

Hi Prasanta,

 

Here is the webrev with suggested changes:  http://cr.openjdk.java.net/~kaddepalli/8190281/webrev05/

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Wednesday, December 13, 2017 10:42 AM
To: Krishna Addepalli <[hidden email]>; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

But there is no compulsion that we need to store getRowCount() in "max". You can store in some other variable and then "max" point to that in the loop.

Regards
Prasanta

On 12/12/2017 9:51 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

The getRowCount() calls l955,956 cannot be removed, since max variable is getting modified in the while loop at l945. There is no guarantee that max will still hold the getRowCount() after the loop exits. So, those calls cannot be removed.

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Tuesday, December 12, 2017 8:08 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

As told, you overlooked l955,956

Regards
Prasanta

On 12/12/2017 7:37 PM, Krishna Addepalli wrote:

Oops! My bad. Created a new webrev here with the correction: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev04/

 

From: Prasanta Sadhukhan
Sent: Tuesday, December 12, 2017 7:05 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

You missed using the variable at l933

Regards
Prasanta

On 12/12/2017 5:21 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Did the change for caching the result of calling “getRowCount()” into a variable, as pointed out by you, and here is the new webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev03/

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Monday, December 11, 2017 7:24 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

 

 

On 12/11/2017 4:16 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Yes, you are right, but as I mentioned earlier, that would need to make one variable declaration for caching before trivial reject case, which I wanted to avoid.

As for the body of getRowCount() it is just returning “visibleNodes.size()”, which shouldn’t be a (performance)problem if called 2 times as I understand.

But, the whole premise of changing getRowCount() <=0  was that it can be overridden and return -ve. Left to present implementation, we would not have needed "less than" check.
So, if we are changing one case because of the above reason, then we cannot forego the 2nd case's problem, as it can have any implementation.

Regards
Prasanta




 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Monday, December 11, 2017 4:02 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi Krishna,

My point was we can call getRowCount() once at first and store the result and use it subsequently. There was no need to call it 2-3 times.

Regards
Prasanta

On 12/11/2017 3:01 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Thanks for pointing out the “getRowCount()==0” check. Modified it to “getRowCount() <= 0” in the new webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev02/

 

As for calling the method twice, you are right that we don’t need to call it twice, but in the interest of having trivial reject case first, and then start the variable declarations, had to let be there to be called twice. Precisely for the reason you stated, it shouldn’t matter if we called it twice.

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Saturday, December 9, 2017 7:54 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi Krishna,

This seems good to me except one thing. You are checking getRowCount() == 0 but there is a chance of test extending VariableHeightLayoutCache and overriding getRowCount to return -ve also as it is an int. In that case, I guess this function will not return -1 which spec mandates [If there are no rows, -1 is returned] so I guess we should check for <=0.
Also, there is no need of calling getRowCount() twice as it will not change between 929, 936.

Regards
Prasanta

On 12/7/2017 4:41 PM, Krishna Addepalli wrote:

Hi Sergey,

 

Per our conversation, I have done the following changes:

1.       Found that the .class size increases by 1kb when streams are used, so reverted the changes related to it.

2.       Moved the “++nextIndex” into the conditional, so that there is no logical change.

Here is the updated webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev01/

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Wednesday, December 6, 2017 2:43 PM
To: [hidden email]
Subject: [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi All,

 

Please review the fix for bug:

Bug: JDK-8190281 https://bugs.openjdk.java.net/browse/JDK-8190281

JDK 10 Webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev00/

 

This bug was created while root causing JDK-8187936, and the following refactoring points have been addressed:

 

1. Line 927: Uninitialized variables, checking for trivial reject case multiple times. 
2. Line 999: Traditional code written to find maximum size of components, which can be done without any local variables and explicit looping by replacing with streams. 
3. Line 1365: Code repetition for differenct conditions, which can be ored together to reduce the repetition. 
4. Line 1482: A large code block gets repeated only because of different values need to be passed in one line. This can be moved to a variable initialization, and the repeating code blocks can be reduced to one. 
5. Line 1505: Variable initialization can be simplified by combining different conditions. 
6. Line 1540: An explicit loop to apply a function over a collection, can be achieved in one line by a forEach construct.  – This is producing some visual artifacts, so ignored.
7. Line 1747: Combine all the trivial reject cases into one condition, and also, a potential bug which increments the "nextIndex" value beyond the length of the containing elements. The increment should happen only if the trivial reject case fails.

 

Thanks,

Krishna

 

 

 

 

 

 

Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

prasanta sadhukhan

+1

Regards
Prasanta
On 12/13/2017 2:12 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Here is the webrev with suggested changes:  http://cr.openjdk.java.net/~kaddepalli/8190281/webrev05/

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Wednesday, December 13, 2017 10:42 AM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

But there is no compulsion that we need to store getRowCount() in "max". You can store in some other variable and then "max" point to that in the loop.

Regards
Prasanta

On 12/12/2017 9:51 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

The getRowCount() calls l955,956 cannot be removed, since max variable is getting modified in the while loop at l945. There is no guarantee that max will still hold the getRowCount() after the loop exits. So, those calls cannot be removed.

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Tuesday, December 12, 2017 8:08 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

As told, you overlooked l955,956

Regards
Prasanta

On 12/12/2017 7:37 PM, Krishna Addepalli wrote:

Oops! My bad. Created a new webrev here with the correction: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev04/

 

From: Prasanta Sadhukhan
Sent: Tuesday, December 12, 2017 7:05 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

You missed using the variable at l933

Regards
Prasanta

On 12/12/2017 5:21 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Did the change for caching the result of calling “getRowCount()” into a variable, as pointed out by you, and here is the new webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev03/

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Monday, December 11, 2017 7:24 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

 

 

On 12/11/2017 4:16 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Yes, you are right, but as I mentioned earlier, that would need to make one variable declaration for caching before trivial reject case, which I wanted to avoid.

As for the body of getRowCount() it is just returning “visibleNodes.size()”, which shouldn’t be a (performance)problem if called 2 times as I understand.

But, the whole premise of changing getRowCount() <=0  was that it can be overridden and return -ve. Left to present implementation, we would not have needed "less than" check.
So, if we are changing one case because of the above reason, then we cannot forego the 2nd case's problem, as it can have any implementation.

Regards
Prasanta




 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Monday, December 11, 2017 4:02 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi Krishna,

My point was we can call getRowCount() once at first and store the result and use it subsequently. There was no need to call it 2-3 times.

Regards
Prasanta

On 12/11/2017 3:01 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Thanks for pointing out the “getRowCount()==0” check. Modified it to “getRowCount() <= 0” in the new webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev02/

 

As for calling the method twice, you are right that we don’t need to call it twice, but in the interest of having trivial reject case first, and then start the variable declarations, had to let be there to be called twice. Precisely for the reason you stated, it shouldn’t matter if we called it twice.

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Saturday, December 9, 2017 7:54 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi Krishna,

This seems good to me except one thing. You are checking getRowCount() == 0 but there is a chance of test extending VariableHeightLayoutCache and overriding getRowCount to return -ve also as it is an int. In that case, I guess this function will not return -1 which spec mandates [If there are no rows, -1 is returned] so I guess we should check for <=0.
Also, there is no need of calling getRowCount() twice as it will not change between 929, 936.

Regards
Prasanta

On 12/7/2017 4:41 PM, Krishna Addepalli wrote:

Hi Sergey,

 

Per our conversation, I have done the following changes:

1.       Found that the .class size increases by 1kb when streams are used, so reverted the changes related to it.

2.       Moved the “++nextIndex” into the conditional, so that there is no logical change.

Here is the updated webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev01/

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Wednesday, December 6, 2017 2:43 PM
To: [hidden email]
Subject: [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi All,

 

Please review the fix for bug:

Bug: JDK-8190281 https://bugs.openjdk.java.net/browse/JDK-8190281

JDK 10 Webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev00/

 

This bug was created while root causing JDK-8187936, and the following refactoring points have been addressed:

 

1. Line 927: Uninitialized variables, checking for trivial reject case multiple times. 
2. Line 999: Traditional code written to find maximum size of components, which can be done without any local variables and explicit looping by replacing with streams. 
3. Line 1365: Code repetition for differenct conditions, which can be ored together to reduce the repetition. 
4. Line 1482: A large code block gets repeated only because of different values need to be passed in one line. This can be moved to a variable initialization, and the repeating code blocks can be reduced to one. 
5. Line 1505: Variable initialization can be simplified by combining different conditions. 
6. Line 1540: An explicit loop to apply a function over a collection, can be achieved in one line by a forEach construct.  – This is producing some visual artifacts, so ignored.
7. Line 1747: Combine all the trivial reject cases into one condition, and also, a potential bug which increments the "nextIndex" value beyond the length of the containing elements. The increment should happen only if the trivial reject case fails.

 

Thanks,

Krishna

 

 

 

 

 

 


Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

Krishna Addepalli

Thanks Prasanta for taking time and reviewing.

@Sergey, could you also review and provide your comments/approval?

 

Krishna

 

From: Prasanta Sadhukhan
Sent: Wednesday, December 13, 2017 2:18 PM
To: Krishna Addepalli <[hidden email]>; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

+1

Regards
Prasanta

On 12/13/2017 2:12 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Here is the webrev with suggested changes:  http://cr.openjdk.java.net/~kaddepalli/8190281/webrev05/

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Wednesday, December 13, 2017 10:42 AM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

But there is no compulsion that we need to store getRowCount() in "max". You can store in some other variable and then "max" point to that in the loop.

Regards
Prasanta

On 12/12/2017 9:51 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

The getRowCount() calls l955,956 cannot be removed, since max variable is getting modified in the while loop at l945. There is no guarantee that max will still hold the getRowCount() after the loop exits. So, those calls cannot be removed.

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Tuesday, December 12, 2017 8:08 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

As told, you overlooked l955,956

Regards
Prasanta

On 12/12/2017 7:37 PM, Krishna Addepalli wrote:

Oops! My bad. Created a new webrev here with the correction: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev04/

 

From: Prasanta Sadhukhan
Sent: Tuesday, December 12, 2017 7:05 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

You missed using the variable at l933

Regards
Prasanta

On 12/12/2017 5:21 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Did the change for caching the result of calling “getRowCount()” into a variable, as pointed out by you, and here is the new webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev03/

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Monday, December 11, 2017 7:24 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

 

 

On 12/11/2017 4:16 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Yes, you are right, but as I mentioned earlier, that would need to make one variable declaration for caching before trivial reject case, which I wanted to avoid.

As for the body of getRowCount() it is just returning “visibleNodes.size()”, which shouldn’t be a (performance)problem if called 2 times as I understand.

But, the whole premise of changing getRowCount() <=0  was that it can be overridden and return -ve. Left to present implementation, we would not have needed "less than" check.
So, if we are changing one case because of the above reason, then we cannot forego the 2nd case's problem, as it can have any implementation.

Regards
Prasanta





 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Monday, December 11, 2017 4:02 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi Krishna,

My point was we can call getRowCount() once at first and store the result and use it subsequently. There was no need to call it 2-3 times.

Regards
Prasanta

On 12/11/2017 3:01 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Thanks for pointing out the “getRowCount()==0” check. Modified it to “getRowCount() <= 0” in the new webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev02/

 

As for calling the method twice, you are right that we don’t need to call it twice, but in the interest of having trivial reject case first, and then start the variable declarations, had to let be there to be called twice. Precisely for the reason you stated, it shouldn’t matter if we called it twice.

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Saturday, December 9, 2017 7:54 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi Krishna,

This seems good to me except one thing. You are checking getRowCount() == 0 but there is a chance of test extending VariableHeightLayoutCache and overriding getRowCount to return -ve also as it is an int. In that case, I guess this function will not return -1 which spec mandates [If there are no rows, -1 is returned] so I guess we should check for <=0.
Also, there is no need of calling getRowCount() twice as it will not change between 929, 936.

Regards
Prasanta

On 12/7/2017 4:41 PM, Krishna Addepalli wrote:

Hi Sergey,

 

Per our conversation, I have done the following changes:

1.       Found that the .class size increases by 1kb when streams are used, so reverted the changes related to it.

2.       Moved the “++nextIndex” into the conditional, so that there is no logical change.

Here is the updated webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev01/

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Wednesday, December 6, 2017 2:43 PM
To: [hidden email]
Subject: [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi All,

 

Please review the fix for bug:

Bug: JDK-8190281 https://bugs.openjdk.java.net/browse/JDK-8190281

JDK 10 Webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev00/

 

This bug was created while root causing JDK-8187936, and the following refactoring points have been addressed:

 

1. Line 927: Uninitialized variables, checking for trivial reject case multiple times. 
2. Line 999: Traditional code written to find maximum size of components, which can be done without any local variables and explicit looping by replacing with streams. 
3. Line 1365: Code repetition for differenct conditions, which can be ored together to reduce the repetition. 
4. Line 1482: A large code block gets repeated only because of different values need to be passed in one line. This can be moved to a variable initialization, and the repeating code blocks can be reduced to one. 
5. Line 1505: Variable initialization can be simplified by combining different conditions. 
6. Line 1540: An explicit loop to apply a function over a collection, can be achieved in one line by a forEach construct.  – This is producing some visual artifacts, so ignored.
7. Line 1747: Combine all the trivial reject cases into one condition, and also, a potential bug which increments the "nextIndex" value beyond the length of the containing elements. The increment should happen only if the trivial reject case fails.

 

Thanks,

Krishna

 

 

 

 

 

 

 

Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

Krishna Addepalli

Hi Sergey,

 

Did you get a chance to review this?

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Wednesday, December 13, 2017 3:01 PM
To: Prasanta Sadhukhan <[hidden email]>; [hidden email]
Subject: RE: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Thanks Prasanta for taking time and reviewing.

@Sergey, could you also review and provide your comments/approval?

 

Krishna

 

From: Prasanta Sadhukhan
Sent: Wednesday, December 13, 2017 2:18 PM
To: Krishna Addepalli <[hidden email]>; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

+1

Regards
Prasanta

On 12/13/2017 2:12 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Here is the webrev with suggested changes:  http://cr.openjdk.java.net/~kaddepalli/8190281/webrev05/

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Wednesday, December 13, 2017 10:42 AM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

But there is no compulsion that we need to store getRowCount() in "max". You can store in some other variable and then "max" point to that in the loop.

Regards
Prasanta

On 12/12/2017 9:51 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

The getRowCount() calls l955,956 cannot be removed, since max variable is getting modified in the while loop at l945. There is no guarantee that max will still hold the getRowCount() after the loop exits. So, those calls cannot be removed.

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Tuesday, December 12, 2017 8:08 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

As told, you overlooked l955,956

Regards
Prasanta

On 12/12/2017 7:37 PM, Krishna Addepalli wrote:

Oops! My bad. Created a new webrev here with the correction: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev04/

 

From: Prasanta Sadhukhan
Sent: Tuesday, December 12, 2017 7:05 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

You missed using the variable at l933

Regards
Prasanta

On 12/12/2017 5:21 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Did the change for caching the result of calling “getRowCount()” into a variable, as pointed out by you, and here is the new webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev03/

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Monday, December 11, 2017 7:24 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

 

 

On 12/11/2017 4:16 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Yes, you are right, but as I mentioned earlier, that would need to make one variable declaration for caching before trivial reject case, which I wanted to avoid.

As for the body of getRowCount() it is just returning “visibleNodes.size()”, which shouldn’t be a (performance)problem if called 2 times as I understand.

But, the whole premise of changing getRowCount() <=0  was that it can be overridden and return -ve. Left to present implementation, we would not have needed "less than" check.
So, if we are changing one case because of the above reason, then we cannot forego the 2nd case's problem, as it can have any implementation.

Regards
Prasanta




 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Monday, December 11, 2017 4:02 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi Krishna,

My point was we can call getRowCount() once at first and store the result and use it subsequently. There was no need to call it 2-3 times.

Regards
Prasanta

On 12/11/2017 3:01 PM, Krishna Addepalli wrote:

Hi Prasanta,

 

Thanks for pointing out the “getRowCount()==0” check. Modified it to “getRowCount() <= 0” in the new webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev02/

 

As for calling the method twice, you are right that we don’t need to call it twice, but in the interest of having trivial reject case first, and then start the variable declarations, had to let be there to be called twice. Precisely for the reason you stated, it shouldn’t matter if we called it twice.

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan
Sent: Saturday, December 9, 2017 7:54 PM
To: Krishna Addepalli [hidden email]; [hidden email]
Subject: Re: <Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi Krishna,

This seems good to me except one thing. You are checking getRowCount() == 0 but there is a chance of test extending VariableHeightLayoutCache and overriding getRowCount to return -ve also as it is an int. In that case, I guess this function will not return -1 which spec mandates [If there are no rows, -1 is returned] so I guess we should check for <=0.
Also, there is no need of calling getRowCount() twice as it will not change between 929, 936.

Regards
Prasanta

On 12/7/2017 4:41 PM, Krishna Addepalli wrote:

Hi Sergey,

 

Per our conversation, I have done the following changes:

1.       Found that the .class size increases by 1kb when streams are used, so reverted the changes related to it.

2.       Moved the “++nextIndex” into the conditional, so that there is no logical change.

Here is the updated webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev01/

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Wednesday, December 6, 2017 2:43 PM
To: [hidden email]
Subject: [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

 

Hi All,

 

Please review the fix for bug:

Bug: JDK-8190281 https://bugs.openjdk.java.net/browse/JDK-8190281

JDK 10 Webrev: http://cr.openjdk.java.net/~kaddepalli/8190281/webrev00/

 

This bug was created while root causing JDK-8187936, and the following refactoring points have been addressed:

 

1. Line 927: Uninitialized variables, checking for trivial reject case multiple times. 
2. Line 999: Traditional code written to find maximum size of components, which can be done without any local variables and explicit looping by replacing with streams. 
3. Line 1365: Code repetition for differenct conditions, which can be ored together to reduce the repetition. 
4. Line 1482: A large code block gets repeated only because of different values need to be passed in one line. This can be moved to a variable initialization, and the repeating code blocks can be reduced to one. 
5. Line 1505: Variable initialization can be simplified by combining different conditions. 
6. Line 1540: An explicit loop to apply a function over a collection, can be achieved in one line by a forEach construct.  – This is producing some visual artifacts, so ignored.
7. Line 1747: Combine all the trivial reject cases into one condition, and also, a potential bug which increments the "nextIndex" value beyond the length of the containing elements. The increment should happen only if the trivial reject case fails.

 

Thanks,

Krishna