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

Circumvent type-instability of deepcopy #13323

Merged
merged 2 commits into from
Apr 6, 2016
Merged

Circumvent type-instability of deepcopy #13323

merged 2 commits into from
Apr 6, 2016

Conversation

timholy
Copy link
Member

@timholy timholy commented Sep 26, 2015

Due to the use of the ObjectIdDict, deepcopy is type-unstable. Obviously this dict is important for avoiding cycles, so it seems the best solution is to prevent the type-instability from propagating.

Ref JuliaImages/Images.jl#356

@timholy
Copy link
Member Author

timholy commented Sep 26, 2015

Hmmm, more complicated than I thought. I guess the fix for this depends on what deepcopy really means. Ref #13124.

@Keno
Copy link
Member

Keno commented Sep 26, 2015

I don't really understand why that issue is related?

@timholy
Copy link
Member Author

timholy commented Sep 26, 2015

The part about equivalence to "serialize" and "deserialize"---that's one way of stating a principle-of-operation for deepcopy, but there might be others. For example, the fact that SharedArrays don't serialize their data would mean you're not actually copying the data.

@timholy timholy closed this Jan 2, 2016
@timholy timholy deleted the teh/deepcopy branch January 2, 2016 13:45
@timholy timholy mentioned this pull request Feb 4, 2016
@timholy timholy mentioned this pull request Apr 5, 2016
@vtjnash
Copy link
Member

vtjnash commented Apr 5, 2016

i think there's a related issue (noted in the serialization code), which is that if the exact type isn't returned, then it might get assigned to a field of a different type causing issues.

i think we should either merge this, or perhaps insert the following slow path to deserialize and deepcopy:

if typeof(elt) != fieldtype(T, i)
  elt = convert(fieldtype(T, i), elt)
end

@timholy timholy restored the teh/deepcopy branch April 5, 2016 17:37
@JeffBezanson
Copy link
Member

I also think we should merge this. Unless I'm missing something, I don't think there is a need for deepcopy to literally give the same result as deserialize. That's just one way to understand the sort of thing it does.
Ideally, serialize of a subarray would still store a subarray, but with the backing array trimmed down. It's pretty bad not to preserve type. Of course now that serialize preserves reference topology even this behavior is questionable, unfortunately.

@timholy
Copy link
Member Author

timholy commented Apr 5, 2016

serialize(::SubArray) does return a SubArray, now, indeed with a trimmed backing array. See #14625 and #14708.

Fix for SharedArrays (ref #6362) coming once local tests finish.

@timholy timholy reopened this Apr 5, 2016
@timholy
Copy link
Member Author

timholy commented Apr 6, 2016

Hmm, the AppVeyor failure seems odd because one can see the final "SUCCESS", but then it's judged as failing. Anything to worry about here?

@vtjnash
Copy link
Member

vtjnash commented Apr 6, 2016

try restarting? it looks all of the tests finished, but one of the subprocesses failed to shutdown

@JeffBezanson
Copy link
Member

👍

@timholy timholy merged commit 5bbe070 into master Apr 6, 2016
@timholy timholy deleted the teh/deepcopy branch April 6, 2016 18:36
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.

4 participants