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

lastOrNull is suboptimal #260

Open
alanrussian opened this issue Dec 23, 2022 · 3 comments
Open

lastOrNull is suboptimal #260

alanrussian opened this issue Dec 23, 2022 · 3 comments

Comments

@alanrussian
Copy link

I believe the current implementation produces two iterators: one for isEmpty and one for last:

  /// The last element, or `null` if the iterable is empty.
  T? get lastOrNull {
    if (isEmpty) return null;
    return last;
  }

Unless I'm missing something here, I think this is suboptimal, especially when using this on a lazy Iterable (for example, giantList.where(someCondition).

@lrhn
Copy link
Member

lrhn commented Jan 11, 2023

It's definitely a trade-off.
We usually try to not create more than one iterator for each operation, but this extension also applies to types that might have an efficient last, in which case doing a complete iteration from start to end is wasteful.

I'm trying to add a lastOrNull to the platform libraries, and I want to be more efficient there, but it's still not easy given the available primitives.

A solution could be to do:

class _Sentinel {
  const _Sentinel();
}
Never _throwSentinel() => throw const _Sentinel();
bool _kTrue(_) => true;

/// ...
  T? get lastOrNull {
    try {
      return lastWhere(_kTrue, orElse: _throwSentinel);
    } on _Sentinel {
      return null;
   }
 }

That uses exceptions for control flow, but it also allows the iterable to be efficient about finding its last element.
It does more computation than just

  try {
    return last;
  } on StateError {
    return null;
  }

but we can't know for sure that the StateError comes from not having any elements. It could also be an internal error in the iterable, which should be reported instead of converted to null.

The primitives are just not available to both allow the iterable to do an efficient last and not create two iterators and not do extra work.

The choice here was that isEmpty is expected to be cheap, because it never needs more than one iteration step.
That expectation can fail on a large iterable with a sparse where filter, or if setting up the iteration takes significant work before getting to the first element.

So, tradeoffs.

@alanrussian
Copy link
Author

I see. I guess we could look at the type (e.g., have a different algorithm for List objects), but that wouldn't necessarily make the right set of tradeoffs in every situation (e.g., BuiltList).

@lrhn
Copy link
Member

lrhn commented Jan 13, 2023

The version I'm hoping to add to the platform libraries relies on is EfficientLengthIterable to detect a lot of cases where isEmpty is guaranteed to be constant time and not create any iterators. It won't handle all of them, there may be non-efficient-length iterables where isEmpty is actually fast (a .take iterable still only needs to look at the first element).
In the other, non-efficient-length, cases, it will probably do a full iteration, even if last could more efficient, which is annoying. The alternative would be doing the same thing as the code here.

It might still be worth having a separate extension for List, to avoid even the type check.

(Another approach is to have extensions

extension <T> on Iterable<T> {
  Iterable<T>? get emptyAsNull => this.isEmpty ? null : this;
}
extension <T> on Iterable<T>? {
  Iterable<T> get nullAsEmpty => this ?? Iterable<T>.empty();
}
// Maybe similar for List/Set/String.

Then instead of lastOrNull, you do

  iterable.emptyAsNull?.last

Doesn't change anything, it's still the same code that we have in this package now, it's just split into two
separate members, so it's not so surprising that it iterates twice.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants