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

Backports for 0.6.1 #23670

Merged
merged 88 commits into from
Oct 7, 2017
Merged

Backports for 0.6.1 #23670

merged 88 commits into from
Oct 7, 2017

Conversation

ararslan
Copy link
Member

This is my first time doing this, so it's probably an enormous mess.

@ararslan ararslan added this to the 0.6.x milestone Sep 11, 2017
@ararslan
Copy link
Member Author

ararslan commented Sep 12, 2017

I need to redo this from scratch. Things to do differently:

  • Cherry pick individual commits; don't use merge commits unless it was a squash merge.
  • Do not include commits from PRs that still have a [needs X] label for any X.

Edit: Redone. Let's see what happens.

@ararslan
Copy link
Member Author

Looks like something was backported incorrectly, likely a bad conflict resolution. Not sure what exactly is causing it but it looks like it's failing somewhere around src/ccall.cpp. @vtjnash, would you be willing to take a look at that and make sure that part looks okay?

@vtjnash
Copy link
Member

vtjnash commented Sep 14, 2017

Added #22161, dropped PR #22472 (096e13d ("Store global variables and function addresses as 32bit offsets") and 054618e ("Fix function base address lookup on Win32 and FreeBSD")),
rebased on top of #23683, and corrected minor typos. Still getting some failures in linalg/generic, linalg/cholesky, misc, linalg/qr, and linalg/diagonal, and misc which you'll need to resolve.

@ararslan
Copy link
Member Author

Ack, I merged #23683 and now there are conflicts. 😑 The linalg stuff may be due to #22138, which Fredrik has since advised against backporting.

jrevels and others added 20 commits September 14, 2017 10:56
removes the global work queue, which allows increasing the precision of cycle detection and resolution, and decreases the need for the threading synchronization lock surrounding inference

updates `inInference` flag usage to be merely a hint for `jl_type_infer` to not bother trying to infer a method (helps avoid accidental infinite recursion over inferring type inference), enable inferring inference

(cherry picked from commit 5847317)
- move call to `short-form-function-loc` inside `parse-assignment`
  this is faster and fixes nested cases like `f(x)=g(x)=1`
- fix a case where an `unexpected "foo"` message was missing quotes

- optimize common case of arglists of comma-separated simple tokens
- some other small optimizations

- put code back in precedence order
- separate parse-Nary and parse-comma so they're easier to read
- cleaner code for parse-unary and parse-factor
Ref #21818
(cherry picked from commit fa8c4d2)
Ref #21832
Squashed for backporting
(cherry picked from commit d86b37c)
(cherry picked from commit 7e7393c)
(cherry picked from commit 6a4acc0)
(cherry picked from commit 93cf6d9)
(cherry picked from commit 2379294)
(cherry picked from commit 7ff6322)
(cherry picked from commit 49abbf3)
(cherry picked from commit eadb699)
(cherry picked from commit 1ce1d75)
* Return socket correctly in socket_reuse_port
* Run reuseport tests only if SO_REUSEPORT is supported.
* Test fix - test for successful addition of workers in distributed test

(cherry picked from commit d3bc329)
Ref #22022
(cherry picked from commit 915910c)
Ref #22133
(cherry picked from commit 994f42c)
* fix spurious newlines in Base.Test printing

* add some doctest to test

(cherry picked from commit 2796b40)
* Extend definition of StridedReshapedArray

* Add test

(cherry picked from commit 92ff1bc)
or 250 characters for a certain broken OS

Ref #22457
(cherry picked from commit 8c1adc8)
@nanosoldier
Copy link
Collaborator

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

@fredrikekre
Copy link
Member

repeat regression is real:
v0.6.0:

julia> @btime repeat($A, inner = $[24], outer=$[1]);
  51.796 μs (611 allocations: 56.66 KiB)

This branch:

julia> @btime repeat($A, inner=$[24], outer=$[1]);
  290.690 μs (3811 allocations: 150.41 KiB)

@@ -415,7 +415,7 @@ _reperr(s, n, N) = throw(ArgumentError("number of " * s * " repetitions " *
n = inner[i]
inner_indices[i] = (1:n) + ((c[i] - 1) * n)
end
R[inner_indices...] = A[c]
fill!(view(R, inner_indices...), A[c])
Copy link
Member

Choose a reason for hiding this comment

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

This must be the cause of the repeat regression, @pabloferz

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a necessary fix, it's meant for the case when A[c] is itself an array. I can think of a way of trying to preserve the speed for the cases when A[c] is not an array, but I don't know what would be the action to take with regards to the backport.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ararslan
Copy link
Member Author

ararslan commented Oct 6, 2017

Pablo's PR bettered the performance on master, so let's see what effect it had here.

@nanosoldier runbenchmarks("misc", vs=":release-0.6")

@mbauman
Copy link
Member

mbauman commented Oct 6, 2017

Could I kindly request one more patch to make this round? #23681 would be nice to backport. I don't think I see it listed and it's a very trivial fix.

@ararslan
Copy link
Member Author

ararslan commented Oct 6, 2017

Can do 👍

@ararslan ararslan changed the title WIP: Backports for 0.6.1 Backports for 0.6.1 Oct 6, 2017
@ararslan
Copy link
Member Author

ararslan commented Oct 6, 2017

Will merge once CI is green (FreeBSD and Circle failures are expected) unless someone beats me to it. DO NOT SQUASH!

@nanosoldier
Copy link
Collaborator

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

@ararslan ararslan merged commit 389b23c into release-0.6 Oct 7, 2017
@ararslan ararslan deleted the aa/backports-0.6 branch October 7, 2017 01:18
@mauro3
Copy link
Contributor

mauro3 commented Oct 7, 2017

Thanks! Now we just need the tag v0.6.1.

@tkelman
Copy link
Contributor

tkelman commented Oct 7, 2017

Where are your pkgeval results and logs? Coverage.jl fails tests on this branch and it's definitely related, the parser changes were breaking and shouldn't be backported.

@StefanKarpinski
Copy link
Member

Are you talking about these parser changes: #22161. They were non-breaking according to @JeffBezanson (who seems like a fairly reliable character), and tagged for backporting by @vtjnash. It's possible they were actually breaking, but if so it was subtle. There was a PkgEval run done, and I'm sure @ararslan can post the results here.

@ViralBShah
Copy link
Member

It would be great to hook up PkgEval the way the benchmarks are hooked up - so it can be triggered from here.

@StefanKarpinski
Copy link
Member

Automating the running of PkgEval is an excellent idea, similar to nanosoldier. I have to confess I have no idea where the PkgEval code even lives. PkgEval.jl doesn’t exist and there seems to be zero documentation of how to run it.

@ararslan
Copy link
Member Author

ararslan commented Oct 7, 2017

https://github.com/JuliaCI/PackageEvaluator.jl

PkgEval is just what the cool kids are calling it. 😉

@ararslan
Copy link
Member Author

ararslan commented Oct 7, 2017

Where are your pkgeval results and logs?

On Nanosoldier, where they ran. I didn't realize I was supposed to post them somewhere, but I can do that.

@ararslan
Copy link
Member Author

ararslan commented Oct 7, 2017

@tkelman
Copy link
Contributor

tkelman commented Oct 7, 2017

Doesn't make much sense that those logs seem to say Coverage.jl passed on this branch. It definitely fails locally against latest release-0.6. That PR caused a change in coverage measurement that made Coverage start failing on nightly when it was merged. Something may be flawed about how the backport test binary that pkgeval used was built.

Or however the logs were converted to csv may be flawed, and marking some failures as passes. Install jq and use the summary.sh script in the PackageEvaluator repo.

@StefanKarpinski
Copy link
Member

PkgEval is just what the cool kids are calling it.

Given that everyone refers to it as PkgEval perhaps we should rename the package?

@ViralBShah
Copy link
Member

To make sure I understand correctly, is it that the parser changes seem to have broken Coverage.jl?

@StefanKarpinski
Copy link
Member

That's what @tkelman's saying, although more details about how and details of what's failing would be helpful. And of course, then there's the question of why PkgEval didn't catch it.

@ararslan
Copy link
Member Author

ararslan commented Oct 8, 2017

It appears that this is what changed in Coverage: JuliaCI/Coverage.jl#136. It seems that change resulted in something more correct for Coverage but because of that broke a test that expected the old behavior. The change seems subtle enough that I doubt pretty much any other package would have noticed. It's still odd that Coverage on PkgEval didn't see it though.

@ararslan
Copy link
Member Author

ararslan commented Oct 8, 2017

Interesting, downloading the binaries I used for running PkgEval and doing Pkg.test("Coverage") manually fails.

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

Successfully merging this pull request may close these issues.