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

WIP: Backports for 1.3-RC2 #32973

Merged
merged 36 commits into from
Sep 8, 2019
Merged

WIP: Backports for 1.3-RC2 #32973

merged 36 commits into from
Sep 8, 2019

Conversation

KristofferC
Copy link
Member

@KristofferC KristofferC commented Aug 20, 2019

Backported PRs:

Non-merged PRs with backport label:

@KristofferC KristofferC added needs nanosoldier run This PR should have benchmarks run on it DO NOT MERGE Do not merge this PR! needs pkgeval Tests for all registered packages should be run with this change labels Aug 20, 2019
@KristofferC
Copy link
Member Author

Since we didn't run benchmarks for 1.2-RC1, running it immediately here instead.

@nanosoldier runbenchmarks(ALL, vs = ":release-1.2")

@nanosoldier
Copy link
Collaborator

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

fredrikekre and others added 20 commits August 25, 2019 10:14
(cherry picked from commit 64be75f)
This allows dropping MbedTLS patches which have been upstreamed.
The order in which configuration options are returned has changed, making a test fail:
make the code more robust by giving priority to more specific options over global ones.

(cherry picked from commit d0b5d98)
The var"##" syntax should be disabled in string interpolation.

Disallow `var` syntax in command interpolations

This is special cased for compatibility. A more general fix would be to
make cmd interpolation syntax exactly the same as string interpolation.

(cherry picked from commit 050160c)
This function operates on values not on types (though it is a bit of
a trap). Also add a test to catch this bug.

(cherry picked from commit aee3fc2)
…2989)

For types that weren't subtypes of AbstractFloat, we used to try
to LU factorize without pivoting and only use pivoting when it failed.
This caused large numerical errors when computing the LU for element
types which promoted to float like numbers such as most integers.
The behavior was never documented and is error prone. Hence, this
PR removes the behavior.

(cherry picked from commit 5af3c2a)
- move the place where --trace-compile outputs precompile statement to a location that catches more cases
- tweak the REPL code to be more amenable to precompilation in light of
- instead of trying to encode all the rules where the precompile emitter
  fails (#28808) just try to precompile and do nothing if it fails.

(cherry picked from commit c0478d8)
Upgrade `doc/make.jl` to introspect branch/tag names from git info

(cherry picked from commit ad8cf8d)
Followup to https://github.com/JuliaLang/julia/pull/31724/files#r317686891; instead of widening the index type dynamically based upon the index vector length, just throw an error in the case where the index type cannot hold all the indices in a CSC format. This previously was an OOB access (and likely segfault) in 1.2, so throwing an error here is not a breaking change -- and throwing an error restores type stability, addressing the performance regression flagged in #32985.

(cherry picked from commit 9725fb4)
@KristofferC
Copy link
Member Author

@nanosoldier runbenchmarks(ALL, vs = ":release-1.2")

@nanosoldier
Copy link
Collaborator

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

@KristofferC
Copy link
Member Author

KristofferC commented Aug 27, 2019

PkgEval: https://gist.github.com/KristofferC/bd078abfa9434082d9100e799280107d

@KristofferC
Copy link
Member Author

SymbolServer passes on master (of the package)

@KristofferC
Copy link
Member Author

SymEngine: something with the LU pivoting that the package cant handle: symengine/SymEngine.jl#177

@KristofferC
Copy link
Member Author

ApproxFunFourier passes locally

@KristofferC
Copy link
Member Author

ApproxFunSingularities passes locally.

@KristofferC
Copy link
Member Author

BSONqs fixed by richiejp/BSONqs.jl@37e5af5.

@KristofferC
Copy link
Member Author

ForwardDiff is some small numeric error. Passes locally with JuliaDiff/ForwardDiff.jl#409.

@KristofferC
Copy link
Member Author

I think we should merge this and tag an RC2.

@oxinabox
Copy link
Contributor

oxinabox commented Sep 6, 2019

I think we should merge this and tag an RC2.

Conceptually, shouldn't every release candidate be something we think could be the release final?
I see the value in adding another that is part way there to check things are fixed, that we think are
but calling it a release candidate when there are half a dozen known blockers is not really very clear.

@KristofferC
Copy link
Member Author

What are the half dozen known blockers?

@oxinabox
Copy link
Contributor

oxinabox commented Sep 6, 2019

I see about half-a-dozen missing ticks in
#32973 (comment)
or is that just out of date?

@KristofferC
Copy link
Member Author

KristofferC commented Sep 6, 2019

The point of the release process is to get a good release in the end and it requires a bit of pragmatism.

I can just check the boxes for you if that will make you happy?

rfourquet and others added 3 commits September 6, 2019 05:52
…sk_ has the lock. (#33159)

* Fix `assert_havelock(::ReentrantLock)` to assert that the _current-task_ has the lock.

Before this commit, new threads would incorrectly believe that they held
a lock on a Condition when they actually didn't, and would allow illegal
operations, e.g. notify:

```julia
julia> c = Threads.Condition()
Base.GenericCondition{ReentrantLock}(Base.InvasiveLinkedList{Task}(nothing, nothing), ReentrantLock(nothing, Base.GenericCondition{Base.Threads.SpinLock}(Base.InvasiveLinkedList{Task}(nothing, nothing), Base.Threads.SpinLock(Base.Threads.Atomic{Int64}(0))), 0))

julia> lock(c)

julia> fetch(Threads.@Spawn Base.assert_havelock(c))  # This should be an ERROR (the new thread doesn't have the lock)

julia> fetch(Threads.@Spawn notify(c))                # This should be an ERROR (the new thread doesn't have the lock)
0

julia> fetch(Threads.@Spawn wait(c))                  # This error should be caught earlier (in assert_havelock).
ERROR: TaskFailedException:
unlock from wrong thread
Stacktrace:
 [1] error(::String) at ./error.jl:33
 [2] unlockall(::ReentrantLock) at ./lock.jl:121
 [3] wait(::Base.GenericCondition{ReentrantLock}) at ./condition.jl:105
 [4] (::var"##19#20")() at ./threadingconstructs.jl:113
```

(The same holds for `@async` as `@spawn`.)

After this change, the assertion works correctly:
```
julia> c = Threads.Condition();

julia> lock(c)

julia> fetch(Threads.@Spawn Base.assert_havelock(c))  # This correctly ERRORs
ERROR: TaskFailedException:
concurrency violation detected
Stacktrace:
 [1] error(::String) at ./error.jl:33
 [2] concurrency_violation() at ./condition.jl:8
 [3] assert_havelock at ./condition.jl:28 [inlined]
 [4] assert_havelock at ./REPL[22]:1 [inlined]
 [5] assert_havelock(::Base.GenericCondition{ReentrantLock}) at ./condition.jl:73
 [6] (::var"##21#22")() at ./threadingconstructs.jl:113
```

Also adds unit test that failed before this commit but now succeeds

* Remove default impl of `assert_havelock`; add `::SpinLock` impl

(cherry picked from commit 784eb57)
@oxinabox
Copy link
Contributor

oxinabox commented Sep 6, 2019

🤷‍♂
Ticking the boxes would make me less happy.

Kristoffer Carlsson added 2 commits September 6, 2019 06:01
@KristofferC KristofferC removed DO NOT MERGE Do not merge this PR! needs nanosoldier run This PR should have benchmarks run on it needs pkgeval Tests for all registered packages should be run with this change labels Sep 6, 2019
@KristofferC
Copy link
Member Author

No matter if we call the merged of this RC2 or not, I will merge this because this PR is getting unwieldy and it needs a fresh start.

@ericphanson
Copy link
Contributor

ericphanson commented Sep 6, 2019

I think the TravelingSalesmanExact timeouts are the same issue as #32167 (ie MOI v0.9 has way slower inference on 1.2 than 1.1). I don’t think anyone uses my package but it’s just a pretty simple JuMP problem and I think it might reflect real regressions that do affect users (who likely haven’t decided to turn simple JuMP solves into a package, so it doesn’t show up on PkgEval). But I guess that’s a 1.2 issue rather than a 1.3 one directly, and I could use compat to restrict to MOI v0.8.

Edit: hmm, the problem persists with MOI v0.8: https://travis-ci.com/ericphanson/TravelingSalesmanExact.jl/builds/126257518

@KristofferC KristofferC merged commit 58803fe into release-1.3 Sep 8, 2019
@delete-merged-branch delete-merged-branch bot deleted the backports-release-1.3 branch September 8, 2019 14:53
@KristofferC
Copy link
Member Author

Yeah, figuring that out would be very nice. It is usually hard to do MWEs for these type of issues though.

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.