Completely Refactor #179
Completely Refactor #179
Conversation
|
@FredericWantiez I think your problems should now be fixed. Could you let me know if it's working okay for you on the AdvancedPS side of things? |
|
Yes, that fixed the issue with the return value ! Thanks |
|
@mhauru @sunxd3 apologies in advance for the rather large PR. When reviewing, I suggest that you ignore everything in |
There was a problem hiding this comment.
I didn't read the last 700 lines of src/copyable_task.jl, nor did read through the deleted tests to see that the new ones cover the same cases. I may come to those later, but figured leaving some comments now would be helpful.
I only have some localised comments and questions, and a couple of design questions. Mostly looks great, thanks so much for doing this!
| end | ||
| return nothing | ||
| end | ||
|
|
There was a problem hiding this comment.
The test cases made me wonder, what happens if I do
f = Libtask.produce
f(0)or something like
using Libtask: produce as prod
prod(0)?
There was a problem hiding this comment.
Good questions. I'll add tests for this (I think it should be fine).
| # Test case 1: stack allocated objects are deep copied. | ||
| @testset "stack allocated objects shallow copy" begin |
There was a problem hiding this comment.
The comment and the name seem conflicting.
There was a problem hiding this comment.
Interesting. This test is just copied over from the old test suite, so I'm not sure what the intention was.
There was a problem hiding this comment.
Here, the accurate description may be "copied by value". Back to the C-implemented version, the behavior is:
- For primitive number types, the value is copied (not deep and not shallow)
- For reference to an object, the reference is copied (not deep and not shallow, just copy the reference by the pointer value)
- Mutable objects allocated to the stack are shallow copied.
But now in the Pure-Julia version, I think only the first item holds. Maybe we should change these lines to copied by value?
| t[1] = 0 | ||
| while true | ||
| produce(t[1]) | ||
| t[1] |
There was a problem hiding this comment.
I don't believe that it does anything -- I'll remove. This is another test that I've copied straight over from the old test suite.
There was a problem hiding this comment.
It's about an issue of the C-implemented version (maybe data initialization and stack size related). We can safely remove them(including the next similar test case) now.
| @test consume(a) == 4 | ||
| end | ||
| end | ||
| @testset "Issue: PR-86 (DynamicPPL.jl/pull/261)" begin |
There was a problem hiding this comment.
Am I missing something, or is this just the same test as above but with more reps?
There was a problem hiding this comment.
Possibly -- I've just left this alone because it appears to be in relationship to a specific issue.
| end | ||
|
|
||
| # Test case 2: Array objects are deeply copied. | ||
| @testset "Array objects deep copy" begin |
There was a problem hiding this comment.
Would it make sense to have a test like this but where the array returned by consume(ttask/a) is mutated by the caller?
There was a problem hiding this comment.
Could you elaborate a little bit?
There was a problem hiding this comment.
I'm wondering about something like
function f()
t = [0 1 2]
while true
produce(t)
t[1] = 1 + t[1]
end
end
ttask = TapedTask(nothing, f)
t = consume(ttask)
@test t[1] == 0
consume(ttask)
@test t[1] == 1
t[1] = -2
consume(ttask)
@test t[1] == -1Or maybe that's not how it works if a copy is made, but then there could be a test to ensure that indeed a copy is made and the above doesn't work.
* Update benchmark.jl * Update benchmark.jl
Co-authored-by: Markus Hauru <[email protected]>
Per #177 this is an attempt to significantly refactor the internals for robustness.
Todo:
taskfield of aTapedTaskought actually to be part of the public interface -- issue to discussion open at Accessingtaskfield of Libtask TapedTask AdvancedPS.jl#113PhiNodes are handled more thoroughly (include a worked example)optimise nested calls (currently throwing away all return type information to get prototype running)see belowLinked Issues:
taskfield of Libtask TapedTask AdvancedPS.jl#113Closes #165
Closes #167
Closes #171
Closes #176
edit: performance optimisation. Everything (as far as I know) is now type-stable where it ought to be, except in the context of nested calls. These are somewhat harder to achieve optimal performance with (you need the IR of the thing that you're going to recurse into in order to figure out the return type of a call-which-might-produce -- since nasty things like recursion exist, it's not completely trivial to do this correctly).