RFC: Add generic copy stagedfunction for all types

[mbauman]

It is a no-op for all immutable types, and tries to call the default constructor for mutable types. Fields are not copied.

Supersedes #9246; see that issue for some discussion of this.

[toivoh]

I'm not comfortable with how this can silently produce the wrong results for types with a non-default constructor. It can also break abstractions; e.g. some types might be written under the assumption that no two instances share the same field values. I would feel a whole lot better if this were opt in.

[JeffBezanson]

That's a good point; I think a default == is much safer. I also wouldn't remove the existing copy fallback for Number etc. since it's good to be able to avoid the staging in those common cases. Maybe a better default copy would just be copy(x)=x ?

[elextr]

Agree with @toivoh this is very dangerous, a user defined constructor is not required to construct an object with fields that match its arguments.

[JeffBezanson]

@mbauman could you explain a bit more about why copy was needed in that gadfly issue?

[andreasnoack]

@JeffBezanson Relates to #9170. A noop copy default would be wrong for matrices.

[mbauman]

@mbauman could you explain a bit more about why copy was needed in that gadfly issue?

Sure, it's just that Gadfly can sometimes mutate some of its aesthetics, and so it copies everything it doesn't know about by default. This can also be fixed within Gadfly, with Gadfly.cat_aes_var!(a::Nothing, b::Date) = b (https://github.com/dcjones/Gadfly.jl/blob/master/src/aesthetics.jl#L237).

But it seems to me that it's so trivial for immutable types to have a noop definition for copy that I figured I'd add it to base.

[toivoh]

A noop copy for immutables seems great, I think.

[mbauman]

Agreed. The question is simply what to do on the mutable branch since we can't restrict the copy method dispatch to immutables. As I see it, there are several options.

• Throw an exception, forcing all mutable types to create their own copy method (if they want one). This is the safest option. I'm leaning towards it myself now, too.
• copy(x) = x. This seems very dangerous and unexpected to me for a mutable type.
• Make an attempt at constructing a new object with the same fields. The most straightforward way to do this is to expect that a constructor with the fields as its args exists (as this PR does). But that constructor could do anything (like dividing all inputs by a constant factor to store things internally in a different unit).

In reality, I think most mutable objects will also want to copy at least one of its fields (in base, I could only remove 2 of the 20 or so mutable methods due to this), so the utility provided here is probably not that great. Perhaps I should just change it to throw an error?

[StefanKarpinski]

Can't we just cheat construct the new object directly the way deserialization does?

[JeffBezanson]

Yes, if we have a default copy for mutables it should use the internal APIs the serializer uses.

@mbauman I find that behavior of gadfly very odd. Your description implies that it might mutate values it doesn't know about, which is hard to make sense of. It also sounds like the code mutates potentially-numberlike values. An alternative might be to wrap unknown values in 1-element mutable cells, or use a Union of all containers gadfly "knows about" to only copy those. cc @dcjones

[StefanKarpinski]

I'm actually in favor of having a default copy implementation. deepcopy now generally Just Works™ as does serialization of arbitrary objects, so I don't see why copy shouldn't also just work out of the box.

[JeffBezanson]

It might be ok, but one reason it might not is that copying just the outer struct seems unlikely to correspond to useful behavior. Needing to copy 2 levels (e.g. struct plus arrays it directly contains) is common. To @andreasnoack 's point, a struct-only copy is just as wrong for matrix types as a no-op copy. A no-op copy might have the advantage of being more obviously wrong.

[StefanKarpinski]

That's true. I guess that's one place where deepcopy is easier to reason about – you know you should copy all of the levels of the structure, so there's no ambiguity.

[mbauman]

Well, I've updated it to use the C API to cheat-construct the object. This is much better (it also now handles undefined fields and circular references), but I agree that its utility is still dubious in many cases.

[mbauman]

As far as why Gadfly tries to copy dates, to be honest, I'm not familiar enough with the implementation to give a more thorough answer.

[elextr]

Even just copying the fields is dangerous, some things are not meant to be copied.

Think about an int field that represents a file descriptor, if it is closed by one object, the other object now has an internal inconsistency.

In general this can occur when a field relates to some external resource, eg pointers returned from C, if the memory is freed by one object the other object has a dangling pointer.

Essentially with this addition every such type needs a copy() to prevent default copying, a very C++ idea that many forget there too.

@mbauman you would likely need to add lots of copy()s to existing Julia code, not just delete a few. That there are so few indicates that for most Julia types the copy-ability of the objects has not been contemplated.

And all packages will also need to check their types for safe copying. And all user code.

This is definitely a breaking change, not just a harmless convenience. And its not like the compiler can provide a deprecation warning or anything.

[mbauman]

Really, all I wanted to add is copy(d::Date)=d as a minimal fix to get Gadfly working. This all just snowballed as folks asked for more.

[JeffBezanson]

In the original gadfly issue I'd say the missing method error for copy did
its job of indicating something amiss.

Maybe Date is another case where we'd want an abstract Atom type above
Number (and Symbol, Date, etc).

[andreasnoack]

What about BigFloats? Isn't the relevant abstraction more Immutable than Atom in this case?

[JeffBezanson]

No Number type should be mutable, even if its internal representation is. So Atom would capture things that are indivisible and immutable based on their semantics, independent of representation. Of course if we could capture immutable with a method definition as well, that would be ideal.

[toivoh]

If we had traits, we could capture immutable with one, right? I guess that the stagedfunction approach could be seen as a DIY trait.

[andreasnoack]

@JeffBezanson. Right now, linear algebra with matrices of BigFloats spend a lot of time on GC because of the allocation. E.g.

julia> A = big(randn(100, 100));
julia> @time lufact!(A);
elapsed time: 0.426773885 seconds (64044648 bytes allocated, 45.59% gc time)

(notice that the factorization is in-place) so from a performance perspective, it might be interesting to experiment with exploiting the mutability of BigFloats.

[JeffBezanson]

That would be a great benchmark for #8699.

[tkelman]

I've seen BigInt-heavy benchmarks also spend 45% of their time in GC. Sorry about the appveyor failures here, unrelated as far as I can tell.

[joehuchette]

OT: I was playing with a JuMP benchmark today that spent ≥50% of it's time in GC; I can distill it and put it somewhere useful if it's of interest.

[mbauman]

I'm closing this on the basis that just copying the outermost structure typically isn't what you want.