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

Interleave eagerly calls MoveNext #694

Closed
Orace opened this issue Nov 13, 2019 · 3 comments
Closed

Interleave eagerly calls MoveNext #694

Orace opened this issue Nov 13, 2019 · 3 comments
Labels
Milestone

Comments

@Orace
Copy link
Contributor

Orace commented Nov 13, 2019

From this part of the code:

// yield the values of each iterator's current position
for (var index = 0; index < iterCount; index++)
{
yield return iteratorList[index].Current;
}

We can see that interleave eagerly have called MoveNext on all the enumerators from all the input sequence before yielding the first value.

This is useless for the ImbalancedInterleaveStrategy.Skip strategy. And this strategy is the only one accessible from the user (the strategies enum is not public).

@atifaziz
Copy link
Member

I don't think that's the case. It removes any iterator whose MoveNext returns false and updates the counters like iterCount accordingly:

case ImbalancedInterleaveStrategy.Skip:
iteratorList.RemoveAt(index); // no longer visit this particular iterator
--iterCount; // reduce the expected number of iterators to visit
--index; // decrement iterator index to compensate for index shifting
--consumedIterators; // decrement consumer iterator count to stay in balance
break;

The loop that yields then uses the updated list of iterators and count.

Your test in PR #696 is flawed.

@Orace
Copy link
Contributor Author

Orace commented Nov 22, 2019

I don't think that's the case. It removes any iterator whose MoveNext returns false and updates the counters like iterCount accordingly

@atifaziz, as usual I was not clear enough in my first attempt to explain what is wrong here.

The subject of this issue is not about the possible multiple calls to MoveNext after a sequence end.
It's about useless call to MoveNext.

Considering this code.

var resultSequence = sequenceA.Interleave(sequenceB);

Expected behavior
One call to MoveNext on an enumerator of resultSequence should result on one call to MoveNext on one enumerator of the input sequence.

var resultSequence = sequenceA.Interleave(sequenceB);
var e = resultSequence.GetEnumerator();
e.MoveNext(); // call to MoveNext on the enumerator of sequenceA
e.MoveNext(); // call to MoveNext on the enumerator of sequenceB
e.MoveNext(); // call to MoveNext on the enumerator of sequenceA
...

Current behavior

One call to MoveNext on an enumerator of resultSequence result on calls to MoveNext on both enumerators of both input sequences.

var resultSequence = sequenceA.Interleave(sequenceB);
var e = resultSequence.GetEnumerator();
e.MoveNext(); // call to MoveNext on the enumerator of sequenceA and sequenceB
e.MoveNext();
e.MoveNext(); // call to MoveNext on the enumerator of sequenceA and sequenceB
...

Here the test I originally committed:

/// <summary>
/// Verify that interleaving do not call enumerators MoveNext method eagerly
/// </summary>
[Test]
public void TestInterleaveDoNoCallMoveNextEagerly()
{
void Code()
{
var sequenceA = Enumerable.Range(1, 1);
var sequenceB = MoreEnumerable.From<int>(() => throw new TestException());
sequenceA.Interleave(sequenceB).Take(1).Consume();
}
Assert.DoesNotThrow(Code);
}

Here what is tested:
If you take one element from the result sequence, only sequenceA should be enumerated.

@atifaziz
Copy link
Member

Okay that makes more sense. I was focusing too much on the loop you quoted initially but you are right, it should avoid iterating other sequences unless needed. Thanks for the clarification.

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

No branches or pull requests

2 participants