Skip to content

Fix Tuple inference regression from #18467 #18572

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

Merged
merged 1 commit into from
Sep 25, 2016

Conversation

TotalVerb
Copy link
Contributor

@TotalVerb TotalVerb commented Sep 18, 2016

This fixes the cat regressions from #18467 (seen in #18569) on my machine. Haven't tested the other ones yet, but I suspect they will be similar. Could someone run nanosoldier?

I wasn't able to come up with a shorter test. I can remove the current one if it's a bad thing to test internal functions.

cc @tkelman @KristofferC @Sacha0

@tkelman
Copy link
Contributor

tkelman commented Sep 18, 2016

@nanosoldier runbenchmarks(ALL, vs = ":master")

@TotalVerb
Copy link
Contributor Author

TotalVerb commented Sep 18, 2016

I've run the sort benchmarks locally and have found no change with this fix. It's possible that I screwed up the testing; we should wait for Nanosoldier. But it's possible that #18467 introduced regressions that are not fixed here, or that a different PR introduced those regressions.

@KristofferC
Copy link
Member

Could run a benchmark on a revert commit.

@TotalVerb
Copy link
Contributor Author

How do we do that? @KristofferC Should I make an alternative PR reverting #18467?

@TotalVerb
Copy link
Contributor Author

I can run benchmarks locally, but it might take a while.

@KristofferC
Copy link
Member

Just make a branch and call nanosoldier in the comment section of the commit. No need for a PR.

@TotalVerb
Copy link
Contributor Author

@KristofferC I don't think I can call nanosoldier. Could you call nanosoldier here? TotalVerb@ea69c12

@KristofferC
Copy link
Member

Has to be on the JuliaLang one I believe. I go to bed now but I can do it tomorrow if no one does it before me.

@TotalVerb
Copy link
Contributor Author

I've run the benchmarks locally for the commit before the revert commit, and I'm currently running them on the revert commit. This should shed some insight as to whether #18467 was the cause.

@nanosoldier
Copy link
Collaborator

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

@TotalVerb
Copy link
Contributor Author

Nanosoldier seems to indicate that the cat benchmark was the only benchmark that changed (I'm treating everything else as noise). So the remaining possibilities are:

I will be able to make a more informed guess which of these is true after the benchmarks complete locally. It is rather unfortunate (unlucky?) that the first benchmark I decided to test with was the cat one.

@TotalVerb
Copy link
Contributor Author

Perhaps it's worth getting nanosoldier to check, but local benchmarking has found that the only memory regression due to #18467 over 1% is ["array","cat",("catnd",5)], which is indeed fixed by this PR.

This means that the large number of other regressions in #18569 are likely caused by a different commit. @tkelman @Sacha0

@TotalVerb
Copy link
Contributor Author

Is this good to merge?

@vtjnash vtjnash merged commit cdce3af into JuliaLang:master Sep 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:inference Type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants