Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Fewer loop checks in SkipWhile - #2669

Closed
jamesqo wants to merge 2 commits into
dotnet:masterfrom
jamesqo:skipw
Closed

Fewer loop checks in SkipWhile#2669
jamesqo wants to merge 2 commits into
dotnet:masterfrom
jamesqo:skipw

Conversation

@jamesqo

@jamesqo jamesqo commented Aug 7, 2015

Copy link
Copy Markdown
Contributor

Separated from PR #2318. Sorry if I seem to be making a lot of PRs today haha, I just want to finish up with all of the changes I've made.

@jamesqo

jamesqo commented Aug 7, 2015

Copy link
Copy Markdown
Contributor Author

Looks like something with ProcessThreadTests.TestStartTimeProperty is failing? This is only happening on Linux debug.

@stephentoub

Copy link
Copy Markdown
Member

looks like something with ProcessThreadTests.TestStartTimeProperty is failing?

Yeah, https://github.com/dotnet/corefx/issues/2613. @Priya91, FYI.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, the old code was easier to read and maintain IMHO. What kind of speedup do you see here? I'd imagine it wouldn't be very significant vs. general overhead of iterators, but I'm prepared to be proven wrong.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new code removes the need for a lifted "yielding" flag an multiple reads of that field. Instead the state machine gets one more state.
This should, in theory, lead to some improvement, but it is indeed curious to know if it indeed made things faster and by how much?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jamesqo, do you have measurements to go along with this change?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stephentoub Yep, here they are (results). Unfortunately, it looks like nguerrera/Maxwe11 are correct; the new code is barely faster than the old code.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've found doing similar work that you really have to do these tests across a range of element types, a range of enumerable types and a range of enumerable sizes. The results can vary considerably.

@stephentoub

Copy link
Copy Markdown
Member

cc: @VSadov

@stephentoub

Copy link
Copy Markdown
Member

Thanks, @jamesqo. Given the discussion thus far and the lack of demonstrable improvements, I'm going to close this out for now. If you're able to show that this has a real positive impact, please do reopen. Thanks!

@karelz karelz modified the milestone: 1.0.0-rtm Dec 3, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants