[Performance] Convert ASLayoutElementSize to atomic #trivial #331
Conversation
|
|
hannahmbanana
changed the title
There was a problem hiding this comment.
Unfortunately this isn't correct.
I don't know that there is any way to achieve this with an atomic.
Now, we could definitely have the storage here be atomic<CGFloat> width, atomic<CGFloat> height etc, and only use a lock when reading/writing .size, and build the size out of those different fields, but I think that would be pretty ugly and probably wouldn't provide a justifying performance improvement.
@appleguy Thoughts?
Source/Layout/ASLayoutElement.mm
Outdated
| _size.height = height; | ||
| ASLayoutElementSize newSize = _size.load(); | ||
| newSize.height = height; | ||
| _size.store(newSize); |
There was a problem hiding this comment.
Unfortunately this breaks atomicity =/ Imagine:
Thread 1: read size A
Thread 2: read size A
Thread 1: write size { A, height = x }
Thread 2: write size { A, width = y }
So we end up with size { A, width = y }, thread 2 overwrote thread 1's change. We should have ended up with size { A, height = x, width = y }.
We have to do the read-update-write all in one operation.
Source/Layout/ASLayoutElement.mm
Outdated
| ASLayoutElementSize newSize = _size.load(); | ||
| newSize.maxWidth = maxLayoutSize.width; | ||
| newSize.maxHeight = maxLayoutSize.height; | ||
| _size.store(newSize); |
There was a problem hiding this comment.
What if _size is changed after it's loaded here but before the new size is stored?
|
@Adlai-Holler @nguyenhuy good catch. I hadn't considered the size writing problem with this. Is it correct that the issue only occurs in the writing methods, and that it would occur for all the writing methods including the ones for scalar values in the struct? I think this would still be worth the win if we take the lock for the writing cases and let reading use the atomic directly. Reads are drastically more common than writes, especially because of relayouts that often get performed on the main thread. |
|
@appleguy Sadly I don't think acquiring the lock only in writing methods will work. @Adlai-Holler Please correct me if I'm wrong. |
|
@nguyenhuy @Adlai-Holler If the atomic were kept and then also the lock used in the writing methods, why would this not work? This is ready for another review. Let me know if you can think of another accessing way that would break this. |
|
@Adlai-Holler @nguyenhuy I've taken a look, and could be missing something, but this seems safe to me...very curious to know what I might have failed to consider. |
|
@appleguy - I don't have merge permissions, could you land this? |
|
Don't worry, I got it :) |
…Group#331) * [Performance] Convert ASLayoutElementSize to atomic * [ASLayoutElementStyle] Put locks around setter methods. * Also add lock to setSize: internal method.
Resolves #318