Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v3]
On Thu, 28 Mar 2024 08:49:44 GMT, drmarmac wrote: >> This PR removes potentially incorrect usages of Stream.peek(). >> The changed code should be covered by the tests that are already present. > > drmarmac has updated the pull request incrementally with two additional > commits since the last revision: > > - Preallocate in SelectedIndicesList.set() > - Remove unused import Thank you for clarifications! Does not look like this is called from a loop, so whatever inefficiency we have is likely to be ok. - Marked as reviewed by angorya (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1430#pullrequestreview-1967473815
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v3]
On Thu, 28 Mar 2024 22:21:32 GMT, drmarmac wrote: >> modules/javafx.controls/src/main/java/javafx/scene/control/ControlUtils.java >> line 166: >> >>> 164: sm.startAtomic(); >>> 165: >>> 166: final List removed = new >>> ArrayList<>(c.getRemovedSize()); >> >> I wonder if we should add a check for 0 size here to bypass all this code >> and unnecessary object allocations if nothing is removed (same for added) > > We certainly could, or maybe use wasRemoved(), but I doubt there will be much > impact. I guess it's preferred to keep changes unrelated to the issue > minimal, so I'd leave it as it is if everyone's ok with that. These short circuiting operations tend to be worth it only if it's a critical path. The GC will collect the allocations very efficiently these days. I didn't check what this code segment is used for, but unless it's looped somewhere, I doubt there will by any change. - PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1543793491
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v3]
On Thu, 28 Mar 2024 17:53:14 GMT, Andy Goryachev wrote: >> drmarmac has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Preallocate in SelectedIndicesList.set() >> - Remove unused import > > modules/javafx.controls/src/main/java/javafx/scene/control/ControlUtils.java > line 166: > >> 164: sm.startAtomic(); >> 165: >> 166: final List removed = new >> ArrayList<>(c.getRemovedSize()); > > I wonder if we should add a check for 0 size here to bypass all this code and > unnecessary object allocations if nothing is removed (same for added) We certainly could, or maybe use wasRemoved(), but I doubt there will be much impact. I guess it's preferred to keep changes unrelated to the issue minimal, so I'd leave it as it is if everyone's ok with that. - PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1543790719
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v3]
On Thu, 28 Mar 2024 08:49:44 GMT, drmarmac wrote: >> This PR removes potentially incorrect usages of Stream.peek(). >> The changed code should be covered by the tests that are already present. > > drmarmac has updated the pull request incrementally with two additional > commits since the last revision: > > - Preallocate in SelectedIndicesList.set() > - Remove unused import modules/javafx.controls/src/main/java/javafx/scene/control/ControlUtils.java line 166: > 164: sm.startAtomic(); > 165: > 166: final List removed = new > ArrayList<>(c.getRemovedSize()); I wonder if we should add a check for 0 size here to bypass all this code if nothing is removed (same for added) - PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1543395945
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v3]
On Thu, 28 Mar 2024 15:48:50 GMT, Andy Goryachev wrote: > a minor question: would it be much easier to understand if the code was > written in conventional procedural style? a straightforward `for` loop? Even if it were (and I'm not sure it would be), it would be a more intrusive change than is required by the fix, so I wouldn't recommend it. - PR Comment: https://git.openjdk.org/jfx/pull/1430#issuecomment-2025771068
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v3]
On Thu, 28 Mar 2024 08:49:44 GMT, drmarmac wrote: >> This PR removes potentially incorrect usages of Stream.peek(). >> The changed code should be covered by the tests that are already present. > > drmarmac has updated the pull request incrementally with two additional > commits since the last revision: > > - Preallocate in SelectedIndicesList.set() > - Remove unused import Looks good. Should be good regarding performance as well. - Marked as reviewed by mhanl (Committer). PR Review: https://git.openjdk.org/jfx/pull/1430#pullrequestreview-1966854737
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v3]
On Thu, 28 Mar 2024 08:49:44 GMT, drmarmac wrote: >> This PR removes potentially incorrect usages of Stream.peek(). >> The changed code should be covered by the tests that are already present. > > drmarmac has updated the pull request incrementally with two additional > commits since the last revision: > > - Preallocate in SelectedIndicesList.set() > - Remove unused import modules/javafx.controls/src/main/java/javafx/scene/control/ControlUtils.java line 40: > 38: import java.util.List; > 39: import java.util.function.IntPredicate; > 40: import java.util.stream.Collectors; and thank you for removing the unused import ... JDK-8289845 - PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1543225967
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v3]
On Thu, 28 Mar 2024 08:49:44 GMT, drmarmac wrote: >> This PR removes potentially incorrect usages of Stream.peek(). >> The changed code should be covered by the tests that are already present. > > drmarmac has updated the pull request incrementally with two additional > commits since the last revision: > > - Preallocate in SelectedIndicesList.set() > - Remove unused import a minor question: would it be much easier to understand if the code was written in conventional procedural style? a straightforward `for` loop? - PR Comment: https://git.openjdk.org/jfx/pull/1430#issuecomment-2025544489
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v3]
On Thu, 28 Mar 2024 08:49:44 GMT, drmarmac wrote: >> This PR removes potentially incorrect usages of Stream.peek(). >> The changed code should be covered by the tests that are already present. > > drmarmac has updated the pull request incrementally with two additional > commits since the last revision: > > - Preallocate in SelectedIndicesList.set() > - Remove unused import Marked as reviewed by kpk (Committer). - PR Review: https://git.openjdk.org/jfx/pull/1430#pullrequestreview-1966109923
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v3]
On Thu, 28 Mar 2024 08:49:44 GMT, drmarmac wrote: >> This PR removes potentially incorrect usages of Stream.peek(). >> The changed code should be covered by the tests that are already present. > > drmarmac has updated the pull request incrementally with two additional > commits since the last revision: > > - Preallocate in SelectedIndicesList.set() > - Remove unused import I also removed a now unused import in ControlUtils.java. - PR Comment: https://git.openjdk.org/jfx/pull/1430#issuecomment-2024697576
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v3]
> This PR removes potentially incorrect usages of Stream.peek(). > The changed code should be covered by the tests that are already present. drmarmac has updated the pull request incrementally with two additional commits since the last revision: - Preallocate in SelectedIndicesList.set() - Remove unused import - Changes: - all: https://git.openjdk.org/jfx/pull/1430/files - new: https://git.openjdk.org/jfx/pull/1430/files/1b285b5b..aed8e7e1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1430=02 - incr: https://webrevs.openjdk.org/?repo=jfx=1430=01-02 Stats: 9 lines in 2 files changed: 3 ins; 2 del; 4 mod Patch: https://git.openjdk.org/jfx/pull/1430.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1430/head:pull/1430 PR: https://git.openjdk.org/jfx/pull/1430
