Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider reducing ImmutableList array copying #1033

Open
ben-manes opened this issue Oct 23, 2022 · 2 comments
Open

Consider reducing ImmutableList array copying #1033

ben-manes opened this issue Oct 23, 2022 · 2 comments
Labels
P3 not scheduled type=performance Related to performance

Comments

@ben-manes
Copy link

This is simply an observation and not a problem, so you are welcome to close as won't fix. I profiled a test build task to see if there its doing anything dumbly inefficient for a faster CI run. Overall it seems fine and GC pauses are okay, but I did notice that the majority of allocations comes from SubjectUtils building ImmutableList instances. See the attached jfr recording and the JMC and JProfiler screenshots below for the cpu and memory hotspots.

A few obvious optimizations might be,

  • concat is often called by methods with names like prependNameIfAny that pass an empty collection. Since ImmutableList.copyOf is given a wrapped iterator it must always copy, even if the only populated iterator is an ImmutableList.
  • append is called with a known size of elements, but the builder is not presized so it must grow to accommodate.
  • It's not clear if ImmutableList is a benefit here as internal data that does not appear to be further modified. You might consider using lightweight wrappers (Arrays.asList, Collections.unmodifiableList) if a safe and non-intrusive change.

Screen Shot 2022-10-23 at 4 33 28 PM

Screen Shot 2022-10-23 at 4 46 52 PM

Screen Shot 2022-10-23 at 4 33 57 PM

Screen Shot 2022-10-23 at 4 46 18 PM

@cpovirk
Copy link
Member

cpovirk commented Oct 24, 2022

Thanks. For years, I expected someone to tell us that all the allocations in Truth were a problem (probably for Android). And no one ever did, and I forgot about it :)

In the early days, most of the profligate allocations happened in the failure case, which we care less about optimizing. But over time (as we noticed that no one complained... :)), we started allocating more in the success case, as you've noticed (1, 2, surely not exhaustive).

This has been one of the reasons that I've tried to discourage people from using Truth in production code, but we have observed that it might be nice if we provided some kind of "mode" for Truth that were suitable for production code.

Plus, if it turns out that we're making every single Truth user's test run a small bit slower, that adds up.

I still don't know that we'll necessarily do anything about this, but I'll try to remember it as a possible project for when I have a little time free to do something simple. Your suggestions look like easy ways to get some of whatever improvement might be available.

@kluever kluever added P3 not scheduled type=performance Related to performance labels Oct 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 not scheduled type=performance Related to performance
Projects
None yet
Development

No branches or pull requests

4 participants
@ben-manes @cpovirk @kluever and others