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

fix #31760, regression in Dict iterate #31762

Merged
merged 1 commit into from
Apr 18, 2019
Merged

fix #31760, regression in Dict iterate #31762

merged 1 commit into from
Apr 18, 2019

Conversation

JeffBezanson
Copy link
Member

This uses 0 as the sentinel instead of nothing. Hopefully somebody can comment on whether this preserves the benefit of the original change.

@JeffBezanson JeffBezanson added this to the 1.2 milestone Apr 18, 2019
@JeffBezanson
Copy link
Member Author

@nanosoldier runbenchmarks("collection" || "iteration", vs = ":master")

@vchuravy
Copy link
Member

Should be fine since the original change was about not doing typemax(Int)+1 iirc

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

@JeffBezanson JeffBezanson merged commit fd0848e into master Apr 18, 2019
@JeffBezanson JeffBezanson deleted the jb/fix31760 branch April 18, 2019 22:11
@KristofferC KristofferC mentioned this pull request Apr 20, 2019
58 tasks
KristofferC pushed a commit that referenced this pull request Apr 20, 2019
@ndinsmore
Copy link
Contributor

@JeffBezanson & @KristofferC would it be prudent to dig in and understand why this fix works, or more specifically why the original version was slow? I ask because the original version was based around similar semantics to iterate and the slowdown you fixed here could be all over the place with implementations of iterate.

@KristofferC
Copy link
Member

KristofferC commented Apr 26, 2019

I think the problem here was basically that there was a nested union with Nothing which doesn't seem to be handled well by the union splitter. Or something like that. I don't think it is all over the place.

@JeffBezanson
Copy link
Member Author

Yes, I think that's it. iterate uses a union of Nothing and a tuple, while this had a Union{Nothing,Int} inside the tuple as well.

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

Successfully merging this pull request may close these issues.

6 participants