Skip to content

Conversation

kcooney
Copy link
Contributor

@kcooney kcooney commented Sep 1, 2025

Description

This can reduce reallocations of the underlying arrays.

Meta

Merge checklist:

  • Pull Request title is short, imperative summary of proposed changes
  • The description documents the what and why
  • If this PR changes behavior or adds a feature, user documentation is updated
  • If this PR touches photon-serde, all messages have been regenerated and hashes have not changed unexpectedly
  • If this PR touches configuration, this is backwards compatible with settings back to v2024.3.1
  • If this PR touches pipeline settings or anything related to data exchange, the frontend typing is updated
  • If this PR addresses a bug, a regression test for it is added

@kcooney kcooney requested a review from a team as a code owner September 1, 2025 16:56
@github-actions github-actions bot added the backend Things relating to photon-core and photon-server label Sep 1, 2025
@kcooney kcooney force-pushed the arraylist-initial-capacity branch 2 times, most recently from da4fc94 to 025eb9a Compare September 1, 2025 19:50
@kcooney kcooney force-pushed the arraylist-initial-capacity branch from 025eb9a to b5d117a Compare September 2, 2025 13:38
@samfreund
Copy link
Member

I'd like to see some data or an argument for why this is necessary. Currently, the size of our array lists isn't really something that tends to be an issue, and it might be better to avoid the added complexity this brings for such a small beenfit.

@spacey-sooty
Copy link
Member

In my mind its small added complexity for small added benefit, seems reasonable.

@mcm001
Copy link
Contributor

mcm001 commented Sep 5, 2025

Only if consistently applied across the codebase. I'd rather we stick to 100% lists without a preset size, unless test data shows that this makes a statistically significant difference on our target platforms.

@kcooney kcooney force-pushed the arraylist-initial-capacity branch from b5d117a to f00904d Compare September 14, 2025 15:04
@kcooney
Copy link
Contributor Author

kcooney commented Sep 14, 2025

@mcm001 @samfreund I made this at the suggestion of @Gold856 (see this comment).
For most of the changes in this PR, I think the added complexity is extremely low. Agreed that the added benefit is hard to measure.

I'd rather we stick to 100% lists without a preset size

Personally I think the changes in #2079 were justified. As of Java 17 the initial capacity of an ArrayList is 10¹, so if the capacity was unset then if there were more than then queued results, PhotonCamera.getAllUnreadResults() would resize two arrays at least once each.

¹ In Java 17 ArrayList instances created via the default constructor share a common empty array; an array of size 10 would be allocated if a single element was inserted into an empty ArrayList.

@mcm001
Copy link
Contributor

mcm001 commented Sep 15, 2025

I'd still like to see some performance metrics if possible. If you see the ArrayList constructor on the hotpath on roborio or older Pi platforms, I'd be more inclined to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Things relating to photon-core and photon-server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants