Skip to content

Conversation

Abubakar-Sattar
Copy link

Upon reviewing the AbstractBaseGraph class in the Guava library, I have identified an opportunity to enhance its performance by reducing redundant object creation. Specifically, in methods that return collections, such as edges(), new AbstractSet instances are created each time the method is called. This can be optimized by caching these collections, assuming the graph's structure remains unchanged.

Proposed Improvement:

Introduce caching for the edges() method to avoid unnecessary object creation. This involves storing the result in a private transient field and returning the cached collection on subsequent calls, provided the graph hasn't been modified.

PS: @cpovirk Do let me know if you want me to change something from it...

Upon reviewing the AbstractBaseGraph class in the Guava library, I have identified an opportunity to enhance its performance by reducing redundant object creation. Specifically, in methods that return collections, such as edges(), new AbstractSet instances are created each time the method is called. This can be optimized by caching these collections, assuming the graph's structure remains unchanged.

Proposed Improvement:

Introduce caching for the edges() method to avoid unnecessary object creation. This involves storing the result in a private transient field and returning the cached collection on subsequent calls, provided the graph hasn't been modified.


P,S: Do let me know if you want me to change something from it...
@cpovirk cpovirk added type=enhancement Make an existing feature better package=graph P4 no SLO labels Feb 3, 2025
@jrtom
Copy link
Member

jrtom commented Jul 24, 2025

@Abubakar-Sattar thanks for your suggestion, and apologies for the late response.

The idea of caching the result (and invalidating that cache if any changes are made) has merit. However:

  1. some elements of the proposed implementation appear to be either incomplete, incorrect, or both
  2. in practice, the benefits may not be worth the increased implementation complexity

Regarding (1), I noticed that

  • you appear to have deleted the incidentEdges() method
  • there are two edges() methods (the existing one and the one you added)
  • you've redeclared at least one other method (edgeCount()) that's already defined
  • you would need to add code to invalidate the cache

So I'm pretty sure this doesn't currently compile, nor work as intended.

Regarding (2), if you know that the graph will not change, you can use ImmutableGraph (https://github.com/google/guava/wiki/GraphsExplained#immutable-implementations). Conversely, if the graph is changing all the time, then caching is not helpful. Therefore, the primary benefit would be in the case where the graph changes infrequently, and I'm not sure that this is a common enough case to warrant the additional implementation complexity.

It's possible I've missed something, but I think I'd need more convincing that this change would be an overall benefit before it would be worth fixing the issues noted under (1).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P4 no SLO package=graph type=enhancement Make an existing feature better
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants