-
Notifications
You must be signed in to change notification settings - Fork 35
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 world age problems when running tests #41
Conversation
Codecov Report
@@ Coverage Diff @@
## master #41 +/- ##
=====================================
Coverage 0% 0%
=====================================
Files 6 6
Lines 1115 1117 +2
=====================================
- Misses 1115 1117 +2
Continue to review full report at Codecov.
|
Is is true that theoretically, we could just |
That was indeed the old bandaid, you can find such a call in |
Yeah, I didn't mean to suggest it as a workaround but just having a note about it in the docs to immediately catch the people going "oh they don't know about |
Makes total sense, great idea. |
Maybe I'm missing something, but wouldn't it be possible to insert a manual world age increment after |
I don't know how to do that, do you? |
I don't -- turns out I didn't actually understand the problem properly (I thought the global world age wasn't incremented properly, but the problem is that the "local" world age is constant despite a new method being defined). So yeah, I can't think of a better way to do this. |
@pfitzseb, I have long suspected that the omission of manual world-age updates is deliberate, but I hadn't really thought carefully until prompted by your question. Now my guess is this: if Julia supported manual updates, what would happen for a compiled function that happened to redefine itself and then trigger a world age update? Or a function that redefined one of its callers? In contrast, if world age only updates when you are at top level, there is no caller (well, there's potentially Note that base "cheats" a bit on this: see, for example, https://github.com/JuliaLang/julia/blob/68db87147f9806db605c4df9f32b0869554cf4b7/src/interpreter.c#L883-L885. Since we can't do that, we have to employ other strategies. |
I now have more of an idea of why I'm struggling with this. Check out this bizarre Julia bug: ...
frame.pc[] = pc
# Handle the return
stmt = pc_expr(frame, pc)
if nstmts == 0 && !isexpr(stmt, :return)
println("aborted")
JuliaInterpreter.show_stackloc(stack, frame, pc)
@show pc frame
ret = Aborted(frame, pc)
@show ret
return ret, nstmts
end
... and that function Aborted(frame::JuliaStackFrame, pc)
println("Aborted constructor")
@show pc frame
lineidx = frame.code.code.codelocs[convert(Int, pc)]
@show lineidx
return Aborted(frame.code.code.linetable[lineidx])
end yet I'm seeing output like
|
Yikes 😨 |
26e04b9
to
4e8e63d
Compare
4e8e63d
to
321fbf3
Compare
OK, this seems to work now, and adds in a fix for generated functions as a bonus. With this the tuple test finishes. However, one negative is that this disables the parallel framework and runs all tests in I'll point out another oddity in the code. |
@test @interpret(isdefined(Toplevel, :Beat)) | ||
@test @interpret(Toplevel.Beat <: Toplevel.DatesMod.Period) | ||
# @test @interpret(isdefined(Toplevel, :Beat)) | ||
# @test @interpret(Toplevel.Beat <: Toplevel.DatesMod.Period) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bizarrely, these seem to break the printing of the TestSet results. I haven't looked into the cause.
Dang, passed on a 2-day old master. I'll update and see what might need fixing. |
Here's an update on the tests with this branch (I had to comment out the `@sync` wrapper and put in a `sleep(300)` to let still-running tests finish). It's not an unequivocal win, there are even some regressions. More work to do, I fear 😢.Julia Version 1.2.0-DEV.344 Commit a44b421138* (2019-02-16 16:52 UTC) Platform Info: OS: Linux (x86_64-linux-gnu) CPU: Intel(R) Xeon(R) CPU E5-2640 v3 @ 2.60GHz WORD_SIZE: 64 LIBM: libopenlibm LLVM: libLLVM-6.0.1 (ORCJIT, haswell) Environment: JULIA_CPU_THREADS = 8 Test run at: 2019-02-16T17:24:21.508Maximum number of statements per lowered expression: 1000000
|
OK, I think this is finally ready to merge. It's failing on nightly because of Debugger's tests; however, presumably Debugger won't pass on nightly without this (specifically, b1f7906). I won't merge right away in case @KristofferC wants to use this as a test case for https://github.com/JuliaLang/Pkg.jl/issues/770. (Unless committing the manifest is "the answer," in which case perhaps that can be closed.)
Compiled that work will be repeated (which can cause some tests to fail). So this doesn't perfectly eliminate spurious faiilures, but it's getting closer. See #44 for discussion.X in the big run below: |
Test file | Passes | Fails | Errors | Broken | Aborted blocks |
---|---|---|---|---|---|
offsetarray | 352 | 0 | 0 | 0 | 3 |
reducedim | 686 | 3 | 0 | 0 | 27 |
bigint | 2155 | 0 | 1 | 0 | 18 |
sorting | 4864 | 0 | 0 | 0 | 8 |
Julia Version 1.2.0-DEV.344
Commit a44b421138* (2019-02-16 16:52 UTC)
Platform Info:
OS: Linux (x86_64-linux-gnu)
CPU: Intel(R) Xeon(R) CPU E5-2640 v3 @ 2.60GHz
WORD_SIZE: 64
LIBM: libopenlibm
LLVM: libLLVM-6.0.1 (ORCJIT, haswell)
Environment:
JULIA_CPU_THREADS = 8
Test run at: 2019-02-17T09:17:51.447
Maximum number of statements per lowered expression: 1000000
Test file | Passes | Fails | Errors | Broken | Aborted blocks |
---|---|---|---|---|---|
ambiguous | 62 | 0 | 0 | 2 | 0 |
subarray | 280 | 0 | 1 | 0 | 1 |
strings/basic | 87290 | 1 | 4 | 0 | 3 |
strings/search | 522 | 0 | 0 | 0 | 0 |
strings/util | 449 | 0 | 0 | 0 | 0 |
strings/io | 12749 | 0 | 0 | 0 | 1 |
strings/types | 2302685 | 0 | 3 | 0 | 4 |
unicode/utf8 | 19 | 0 | 0 | 0 | 0 |
core | X | X | X | X | X |
worlds | ☠️ | ☠️ | ☠️ | ☠️ | ☠️ |
keywordargs | 133 | 0 | 1 | 0 | 0 |
numbers | 1518443 | 27 | 284 | 0 | 6 |
subtype | X | X | X | X | X |
char | ☠️ | ☠️ | ☠️ | ☠️ | ☠️ |
triplequote | 28 | 0 | 0 | 0 | 0 |
intrinsics | 44 | 0 | 0 | 0 | 0 |
dict | 144290 | 1 | 1 | 0 | 8 |
hashing | X | X | X | X | X |
iobuffer | 200 | 0 | 0 | 0 | 1 |
staged | 57 | 3 | 0 | 0 | 0 |
offsetarray | X | X | X | X | X |
arrayops | ☠️ | ☠️ | ☠️ | ☠️ | ☠️ |
tuple | 488 | 0 | 1 | 0 | 0 |
reduce | 8465 | 0 | 3 | 0 | 4 |
reducedim | X | X | X | X | X |
abstractarray | ☠️ | ☠️ | ☠️ | ☠️ | ☠️ |
intfuncs | 4404 | 0 | 15 | 0 | 1 |
simdloop | X | X | X | X | X |
vecelement | X | X | X | X | X |
rational | 97493 | 0 | 29 | 0 | 2 |
bitarray | 899682 | 0 | 0 | 0 | 9 |
copy | 493 | 11 | 7 | 0 | 0 |
math | 1505698 | 0 | 48 | 0 | 5 |
fastmath | 907 | 3 | 3 | 0 | 0 |
functional | 95 | 0 | 0 | 0 | 0 |
iterators | 1568 | 0 | 4 | 0 | 2 |
operators | 12928 | 0 | 0 | 0 | 1 |
path | 274 | 0 | 0 | 12 | 2 |
ccall | X | X | X | X | X |
parse | 10281 | 20 | 1 | 0 | 1 |
loading | 143633 | 289 | 1 | 0 | 10 |
bigint | X | X | X | X | X |
sorting | X | X | X | X | X |
spawn | X | X | X | X | X |
backtrace | 5 | 9 | 12 | 1 | 0 |
exceptions | 28 | 19 | 6 | 0 | 0 |
file | X | X | X | X | X |
read | X | X | X | X | X |
version | 2468 | 0 | 0 | 0 | 1 |
namedtuple | 161 | 0 | 8 | 1 | 0 |
mpfr | 758 | 0 | 45 | 0 | 0 |
broadcast | 416 | 0 | 7 | 0 | 2 |
complex | 8249 | 0 | 1 | 2 | 1 |
floatapprox | 49 | 0 | 0 | 0 | 0 |
reflection | X | X | X | X | X |
regex | 38 | 0 | 1 | 0 | 0 |
float16 | 124 | 0 | 0 | 0 | 0 |
combinatorics | 95 | 0 | 3 | 0 | 1 |
sysinfo | 2 | 0 | 0 | 0 | 0 |
env | X | X | X | X | X |
rounding | 112496 | 0 | 1 | 0 | 2 |
ranges | 12108707 | 2 | 209 | 327749 | 7 |
mod2pi | 80 | 0 | 0 | 0 | 0 |
euler | 8 | 0 | 4 | 0 | 4 |
show | X | X | X | X | X |
client | 0 | 3 | 0 | 0 | 0 |
errorshow | X | X | X | X | X |
sets | 739 | 0 | 1 | 0 | 1 |
goto | X | X | X | X | X |
llvmcall | X | X | X | X | X |
llvmcall2 | 6 | 0 | 0 | 0 | 0 |
grisu | X | X | X | X | X |
some | 64 | 0 | 0 | 0 | 0 |
meta | X | X | X | X | X |
stacktraces | X | X | X | X | X |
docs | X | X | X | X | X |
misc | X | X | X | X | X |
threads | X | X | X | X | X |
enums | 84 | 3 | 3 | 0 | 0 |
cmdlineargs | X | X | X | X | X |
int | 10545 | 0 | 184 | 0 | 1 |
checked | 1102 | 0 | 9 | 0 | 0 |
bitset | 192 | 0 | 0 | 0 | 0 |
floatfuncs | 116 | 0 | 1 | 0 | 1 |
boundscheck | X | X | X | X | X |
error | 31 | 0 | 0 | 0 | 0 |
cartesian | 7 | 0 | 0 | 0 | 0 |
osutils | 42 | 0 | 0 | 0 | 0 |
channels | ☠️ | ☠️ | ☠️ | ☠️ | ☠️ |
iostream | 11 | 0 | 2 | 0 | 0 |
secretbuffer | 17 | 0 | 0 | 0 | 0 |
specificity | X | X | X | X | X |
reinterpretarray | 128 | 0 | 0 | 0 | 1 |
syntax | X | X | X | X | X |
logging | 117 | 2 | 0 | 0 | 0 |
missing | 406 | 0 | 0 | 1 | 1 |
asyncmap | X | X | X | X | X |
SparseArrays/higherorderfns | 7031 | 64 | 0 | 73 | 8 |
SparseArrays/sparse | ☠️ | ☠️ | ☠️ | ☠️ | ☠️ |
SparseArrays/sparsevector | 9884 | 0 | 1 | 0 | 5 |
Pkg/resolve | 128 | 1 | 10 | 0 | 1 |
LinearAlgebra/triangular | 33194 | 0 | 0 | 0 | 2 |
LinearAlgebra/qr | ☠️ | ☠️ | ☠️ | ☠️ | ☠️ |
LinearAlgebra/dense | 7692 | 0 | 14 | 0 | 7 |
LinearAlgebra/matmul | X | X | X | X | X |
LinearAlgebra/schur | 390 | 0 | 0 | 0 | 1 |
LinearAlgebra/special | 2863 | 0 | 0 | 0 | 6 |
LinearAlgebra/eigen | 406 | 0 | 0 | 0 | 2 |
LinearAlgebra/bunchkaufman | 5145 | 0 | 0 | 0 | 1 |
LinearAlgebra/svd | X | X | X | X | X |
LinearAlgebra/lapack | ☠️ | ☠️ | ☠️ | ☠️ | ☠️ |
LinearAlgebra/tridiag | 1222 | 0 | 0 | 0 | 2 |
LinearAlgebra/bidiag | 1946 | 0 | 0 | 0 | 1 |
LinearAlgebra/diagonal | 1619 | 0 | 0 | 0 | 2 |
LinearAlgebra/cholesky | 2194 | 0 | 0 | 0 | 1 |
LinearAlgebra/lu | 1182 | 0 | 3 | 0 | 1 |
LinearAlgebra/symmetric | 2076 | 0 | 0 | 0 | 2 |
LinearAlgebra/generic | 436 | 0 | 2 | 0 | 3 |
LinearAlgebra/uniformscaling | X | X | X | X | X |
LinearAlgebra/lq | 1253 | 0 | 0 | 0 | 1 |
LinearAlgebra/hessenberg | 40 | 0 | 0 | 0 | 0 |
LinearAlgebra/blas | 628 | 0 | 0 | 0 | 1 |
LinearAlgebra/adjtrans | 253 | 0 | 0 | 0 | 1 |
LinearAlgebra/pinv | 288 | 0 | 0 | 0 | 1 |
LinearAlgebra/givens | 1840 | 0 | 0 | 0 | 1 |
LinearAlgebra/structuredbroadcast | 408 | 0 | 0 | 0 | 2 |
LibGit2/libgit2 | 57 | 0 | 80 | 0 | 0 |
Dates/accessors | 7723858 | 0 | 0 | 0 | 4 |
Dates/adjusters | 3147 | 0 | 0 | 0 | 4 |
Dates/query | 988 | 0 | 0 | 0 | 0 |
Dates/periods | 679 | 0 | 2 | 0 | 1 |
Dates/ranges | 349123 | 0 | 0 | 0 | 5 |
Dates/rounding | 296 | 0 | 0 | 0 | 0 |
Dates/types | 169 | 0 | 2 | 0 | 0 |
Dates/io | 100 | 22 | 166 | 0 | 0 |
Dates/arithmetic | 318 | 0 | 0 | 0 | 0 |
Dates/conversions | 160 | 0 | 0 | 0 | 0 |
Base64 | 1015 | 0 | 0 | 0 | 1 |
CRC32c | 658 | 0 | 6 | 0 | 0 |
DelimitedFiles | X | X | X | X | X |
FileWatching | X | X | X | X | X |
Future | 0 | 0 | 0 | 0 | 0 |
InteractiveUtils | 96 | 3 | 7 | 0 | 4 |
Libdl | X | X | X | X | X |
Logging | 35 | 1 | 0 | 0 | 1 |
Markdown | X | X | X | X | X |
Mmap | ☠️ | ☠️ | ☠️ | ☠️ | ☠️ |
Printf | 701 | 38 | 0 | 0 | 0 |
Profile | 15 | 0 | 0 | 0 | 2 |
REPL | 995 | 0 | 0 | 5 | 0 |
Random | X | X | X | X | X |
SHA | X | X | X | X | X |
Serialization | X | X | X | X | X |
Sockets | X | X | X | X | X |
Statistics | 606 | 0 | 1 | 0 | 4 |
SuiteSparse | 778 | 0 | 0 | 0 | 0 |
Test | X | X | X | X | X |
UUIDs | 22 | 0 | 0 | 0 | 0 |
Unicode | ☠️ | ☠️ | ☠️ | ☠️ | ☠️ |
Also fixes an issue in which next_until! failed to store the final pc.
This reworks prepare_toplevel so that it produces a list of frames that can be executed in order. In conjunction with another addition, through_methoddef_or_done!, one can run each frame sequentially up to the point of a new method definition, after which control can return to top-level. This allows the world age to update. While this places more burden on the top-level evaluator (one now requires a nest of two loops), this resolves the difficulties with previous strategies. It fixes a test previously marked as broken.
Also adds some line number info in prepare_toplevel to assist testing
This avoids the need for a macroexpand in prepare_toplevel, and therefore preserves the block organization of the original code. This also fixes a bug in optimize! which would inappropriately extract "inner methods" from :toplevel expressions.
Also fixes a bug when trying to run a :thunk expr in Compiled mode.
The problem here is that lowering can depend on the result of a previous frame. Consider an example in Julia's test/offsetarray.jl: stuff module NewModule using Test atsign-test_throws blah blah end That `atsign-test_throws` cannot be lowered until the `using Test` has been executed inside `NewModule`.
f63f241
to
2c64810
Compare
I will merge this once tests pass. |
Restarted test to pickup Debugger fix |
There will be an output difference in the Debugger tests, but let's deal with that separately. https://travis-ci.org/JuliaDebug/JuliaInterpreter.jl/jobs/494967019 |
👍 |
I've updated the table in #13. Also check out the note about the crashes:
|
This WiP solves the world age problems by an inversion of control: rather than having
lower_incrementally(f, mod, expr)
recurse intoexpr
, lower its individual pieces, and then running the resulting frames withf
, this redesignsprepare_toplevel
so that it splitsexpr
into frames and returns it as a task list. The caller is then responsible for running each of them. While this is extra work for the caller, the big advantage is that if you do this in top-level you can ensure that there is a world age update after each new method definition. (This is achieved just by returning "prematurely" after each 3-arg:method
expr is encountered.)The core tests pass, but I'm having surprising difficulty rewriting
juliatests.jl
to use this. I may have to pivot to other things for the remainder of the day, so please don't merge yet. (I did want people to know this is coming---it's a pretty disruptive change.)Bonus: includes new docs!