<Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java
Sergey Bylokhov
Sergey.Bylokhov at oracle.com
Mon Dec 18 23:35:21 UTC 2017
Looks fine.
On 13/12/2017 00:48, Prasanta Sadhukhan wrote:
> +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/
>> <http://cr.openjdk.java.net/%7Ekaddepalli/8190281/webrev05/>
>>
>> Thanks,
>>
>> Krishna
>>
>> *From:*Prasanta Sadhukhan
>> *Sent:* Wednesday, December 13, 2017 10:42 AM
>> *To:* Krishna Addepalli <krishna.addepalli at oracle.com>;
>> swing-dev at openjdk.java.net
>> *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 <krishna.addepalli at oracle.com>
>> <mailto:krishna.addepalli at oracle.com>; swing-dev at openjdk.java.net
>> <mailto:swing-dev at openjdk.java.net>
>> *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/
>> <http://cr.openjdk.java.net/%7Ekaddepalli/8190281/webrev04/>
>>
>> *From:*Prasanta Sadhukhan
>> *Sent:* Tuesday, December 12, 2017 7:05 PM
>> *To:* Krishna Addepalli <krishna.addepalli at oracle.com>
>> <mailto:krishna.addepalli at oracle.com>;
>> swing-dev at openjdk.java.net <mailto:swing-dev at openjdk.java.net>
>> *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/
>> <http://cr.openjdk.java.net/%7Ekaddepalli/8190281/webrev03/>
>>
>> Thanks,
>>
>> Krishna
>>
>> *From:*Prasanta Sadhukhan
>> *Sent:* Monday, December 11, 2017 7:24 PM
>> *To:* Krishna Addepalli <krishna.addepalli at oracle.com>
>> <mailto:krishna.addepalli at oracle.com>;
>> swing-dev at openjdk.java.net <mailto:swing-dev at openjdk.java.net>
>> *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 <krishna.addepalli at oracle.com>
>> <mailto:krishna.addepalli at oracle.com>;
>> swing-dev at openjdk.java.net
>> <mailto:swing-dev at openjdk.java.net>
>> *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/
>> <http://cr.openjdk.java.net/%7Ekaddepalli/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
>> <krishna.addepalli at oracle.com>
>> <mailto:krishna.addepalli at oracle.com>;
>> swing-dev at openjdk.java.net
>> <mailto:swing-dev at openjdk.java.net>
>> *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/
>> <http://cr.openjdk.java.net/%7Ekaddepalli/8190281/webrev01/>
>>
>> Thanks,
>>
>> Krishna
>>
>> *From:* Krishna Addepalli
>> *Sent:* Wednesday, December 6, 2017 2:43 PM
>> *To:* swing-dev at openjdk.java.net
>> <mailto:swing-dev at openjdk.java.net>
>> *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/
>> <http://cr.openjdk.java.net/%7Ekaddepalli/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
>>
>
--
Best regards, Sergey.
More information about the swing-dev
mailing list