lacuna / bifurcan

functional, durable data structures
MIT License
967 stars 51 forks source link

`DirectedGraph#edge(V, V)` can be improved by removing the `Optional` allocation #12

Closed michaelamaura closed 5 years ago

michaelamaura commented 5 years ago

Currently, DirectedGraph#edge relies on Map#get(K)which returns an optional.

This could be improved by using Map#get(K, V)with a default value - especially in highly frequently used graph traversals this could save man allocations.

So instead of:

  @Override
  public E edge(V from, V to) {

    Map m = out.get(from).orElseThrow(() -> new IllegalArgumentException("no such edge"));
    Object e = m.get(to, DEFAULT);

    if (e == DEFAULT) {
      throw new IllegalArgumentException("no such edge");
    } else {
      return (E) e;
    }
  }

It could be something like:

  @Override
  public E edge(V from, V to) {

    Map m = out.get(from, DEFAULT_MAP)
    if (m == DEFAULT) {
      throw new IllegalArgumentException("no such edge");
   }

    Object e = m.get(to, DEFAULT);

    if (e == DEFAULT) {
      throw new IllegalArgumentException("no such edge");
    } else {
      return (E) e;
    }
  }
ztellman commented 5 years ago

The JVM's escape analysis exists for exactly these sorts of situations, and should prevent any actual allocations from occurring. I'm willing to consider benchmarks that prove real overhead here, but otherwise I'm going to err on the side of keeping the code simpler.

ztellman commented 5 years ago

Closing, pending justifying benchmarks.