-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
container: remove unnecessary slice copy from Container.Remove #4585
base: develop
Are you sure you want to change the base?
Conversation
Test failure is due to a data race. Once #4584 goes in, the test will pass. |
That's merged now. I merged that commit into this one. Will approve and merge once tests pass. |
Hmm. The same test is failing on macOS? |
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. |
62116a1
to
6572e87
Compare
Still failing. Neat. I’ll look later. |
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.) |
Good summary, thanks. I'm not sure if there is a contract about who owns the field that I can point to. I guess whichever gives us the test pass and safety whilst being most efficient at runtime? |
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. |
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) |
Ok, I will put together a pull request so we can see what that looks like in practice and decide whether we like it. |
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. |
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 Let's chat on Friday |
Description:
Simple optimization: No need to make a copy, just operate in-place.
Checklist: