container: remove unnecessary slice copy from Container.Remove

[josharian]

Description:

Simple optimization: No need to make a copy, just operate in-place.

Checklist:

• Tests included.
• Lint and formatter run with no errors.
• Tests all pass.

[coveralls]

coverage: 64.615% (-0.004%) from 64.619%
when pulling 6572e87 on josharian:josh/container-remove-no-copy
into 85644c2 on fyne-io:develop.

[josharian]

Test failure is due to a data race. Once #4584 goes in, the test will pass.

[Jacalz]

That's merged now. I merged that commit into this one. Will approve and merge once tests pass.

[Jacalz]

Hmm. The same test is failing on macOS?

[josharian]

Unfortunately, GitHub tests the exact commit (which is unchanged after the merge), not "what would be in develop after merging". I'll rebase my commit and then the tests will pass.

[josharian]

Still failing. Neat. I’ll look later.

[josharian]

OK, the problem is confusion over who owns the objects slice.

The code is inconsistent.

In some places (e.g. Remove), it appears that the container maintains its own copy of the objects slice. The test enforces that by concurrently ranging over a slice while asking the container to modify it. The test only makes sense if containers are supposed to make and maintain their own copy of their objects slice.

In other places (e.g. NewContainer, Add), it appears that the container simply shares a reference to the objects slice, and modifies it as it sees fit.

We should probably decide about this, make it consistent throughout, and document it. (Either way, this optimization will be able to stay: Either NewContainerWithLayout will make a copy, so the test will pass, or the test needs to make its own copy to range over as it asks the container to modify the shared slice.)

[andydotxyz]

Good summary, thanks. I'm not sure if there is a contract about who owns the field that I can point to.
What we do know is that developers can update it and then call Layout (or Refresh) (which is something we need to look at - perhaps we should add an equivalent to the new BaseWidget.SetFieldsAndRefresh like SetObjectsAndLayout?).

I guess whichever gives us the test pass and safety whilst being most efficient at runtime?

[dweymouth]

This goes back to the idea that the publicly-exported slices are problematic when it comes to thread safety as they can be updated any time from user code, even if we are holding an internal lock. Not sure that much can really be done here.

[andydotxyz]

This goes back to the idea that the publicly-exported slices are problematic when it comes to thread safety as they can be updated any time from user code, even if we are holding an internal lock. Not sure that much can really be done here.

That is indeed the crux of where a public API change will be needed (or documented ?!) to resolve all of the race possibilities.
Ideally we can demonstrate how to avoid them with non-breaking changes in our apps - the breaking Api change is to be put off until we have 100% confidence in our solution!

[dweymouth]

In some places (e.g. Remove), it appears that the container maintains its own copy of the objects slice. The test enforces that by concurrently ranging over a slice while asking the container to modify it. The test only makes sense if containers are supposed to make and maintain their own copy of their objects slice.

Regarding this, I think it makes more sense for the Container to "own" the Objects slice, and for the tests to be updated to reflect that. (I put "own" in quotes, because as a public field, it's really "owned" by user code in a sense, but we can document how users should safely access the field)

[josharian]

Regarding this, I think it makes more sense for the Container to "own" the Objects slice, and for the tests to be updated to reflect that.

Ok, I will put together a pull request so we can see what that looks like in practice and decide whether we like it.

[josharian]

we can document how users should safely access the field

I looked at this a bit, and I would describe my current status as "mild despair". I don't think there is a safe way for users to safely access the field directly, at all, but doing everything indirectly involves adding a lot of new API surface area. The smallest option I see would be a getter and a setter for wholesale replacement.

I'm torn. I could add some band-aids, like a few docs and removing the unsafe usage from the test and adding more locking...but that also feels like potentially a waste of effort. I think I might just want to wait for the next live contributor discussion to talk it out a bit.

[andydotxyz]

I don't think there is a safe way for users to safely access the field directly, at all,

That may indeed be the case - and what we are looking at is how we can provide a way to guarantee the safety for field access when it matters. Perhaps mirroring the SetFieldsAndRefresh recently added...

Let's chat on Friday