Make Envelope immutable

[cmhodgson]

Envelopes are generally used as if they are immutable, at least within the core geometry classes, however, because they are mutable, they are copied whenever they are returned - even in GetEnvelopeInternal(). This is unnecessary allocation as the envelopes could be shared directly if they were defined to be immutable.

A MutableEnvelope should probably also be provided, for those cases where the mutability of the envelope is used, eg. computing envelopes using expandToInclude(), etc.

This obviously has broad reaching effects that will need to be considered.

[dr-jts]

Agreed. But how to get there from here?

If and when JTS goes to 2.0, there will for sure be less mutability of simple objects.

[cmhodgson]

Do you see it as too big of a breaking change to just remove the mutability functions from Envelope and push them all into a MutableEnvelope subclass? Yes it will break a lot of code but anyone who needs mutable envelopes can just search and replace the class and call: new MutableEnvelope(envelope) or something like that as needed. This also gives users a moment to think about their own usage and whether they really need mutable envelopes or not.

Perhaps it comes down to, do we really want to commit to such a massive all-at-once re-design as 2.0 seems like it might be? Or does it make sense to take smaller steps in that direction, and could this change be one of them?

[dr-jts]

Do you see it as too big of a breaking change to just remove the mutability functions from Envelope and push them all into a MutableEnvelope subclass?

Yes, I think this is probably to big a breakage. It would break an awful lot of code right away. And it can be very painful to have to open up the calling code base and S&R everything. The time do have done this was with the big locationtech switcheroo, I guess...

EDIT: on the other hand, the advantages are great. Maybe this could be a celebratory upgrade in JTS 1.20...!

Perhaps it comes down to, do we really want to commit to such a massive all-at-once re-design as 2.0 seems like it might be? Or does it make sense to take smaller steps in that direction, and could this change be one of them?

That's a point to consider, for sure. It of course assumes that the 2.0 design is understood well enough that the early steps are in fact in the right direction! This one seems fairly certain, however.

[jnh5y]

Relative to the locationtech package name migration, I was able to get 95% of that down to two commands: https://github.com/locationtech/jts/blob/master/MIGRATION.md ;)

Maybe we can think up a clever approach, but yeah, seems like this'll be a breaking change to "clean things up".

I wonder if we could add a new method to return immutable envelopes as a way to help?

[dr-jts]

I wonder if we could add a new method to return immutable envelopes as a way to help?

I think perhaps a method to return a MutableEnvelope?

Anyway, thinking about it more, there should be fairly few places where Envelopes are mutated.

We will need to add non-mutating methods in Envelope to replace things like expandToInclude.

Would be worth a spike to see how much work JTS needs. (Ironically, it is probably a relatively heavy user of envelope mutation).

[dr-jts]

How about this for a near-term solution? Make Envelope "library-immutable", so that client software is assured that the library will not mutate envelopes, and that "well-behaved" software won't either.

Do this by:

• Deprecate the small set of mutating methods (init, expand*, and setToNull)
• Provide an EnvelopeBuilder class which provides those methods and a small subset of useful other ones (such as creators over various kinds of lists of Coordinates)
• Expunge the mutating methods from JTS usage of Envelope (this should be fairly easy)

I think I prefer the Builder pattern to the Immutable/Mutable class hierarchy - it's simpler and less likely to be misused.

Thoughts? @cmhodgson @jnh5y @jodygarnett

[cmhodgson]

So for my usage, it's not that I'm worried that JTS will mutate an envelope, it is that I don't want it to have to allocate a new one whenever I ask it for a geometry's envelope. The geometry already caches the envelope, I want to use that one instead of caching another one in eg. an index structure. In order to get there we have to be sure that a user cannot mutate the envelope we gave them.

That said, deprecating the mutations and adding a builder sounds like a good first step. I also like the clarity of the builder pattern in combination with the assumption of immutability. I just don't think this step gets us any actual reduction in allocations, yet.

[dr-jts]

I see your point. Even with the deprecations and the new API, JTS will have to keep defensively copying Envelopes until the deprecated methods are removed.

I think we can be fairly aggressive about that - say remove the methods and the copying after two versions?

[jodygarnett]

It is tricky because most downstream code in geotools and udig make use of extensions of Envelope; so we have to pay attention to not only the public methods and fields but the protected ones as well

[dr-jts]

we have to pay attention to not only the public methods and fields but the protected ones as well

It's ok, there are no protected members in Envelope.