Make objects conform to NSLocking #851

Adlai-Holler · 2018-03-24T03:50:51Z

Display node, layout spec, element style now conform to NSLocking. They use the same __instanceLock__ as before, but they can be publicly locked and unlocked.
Subclasses (both inside and outside the framework) can use the existing lock, rather than adding their own locks.
Users can lock/unlock nodes to group changes.
ASThread.h (public) has useful C++-esque locking macros.
Message-sends are not the end of the world. The readability and safety benefits here are worth the trivial amount of overhead.
NSLocking is nice because we can retain the lock object, avoiding crashes we had before where an object died while someone else held its lock.
New macros:
- ASLockScope(id<NSLocking> l) retains and adds the lock to the current scope.
- ASLockScopeSelf locks self for the current scope. Skips retain (self is safe.)
- ASLocked(id<NSLocking> l, expr) Evaluates to the expr while holding the lock.
- ASLockedSelf(expr) Evaluates to the expr while using self as the lock (skip retain).
- ASCompareAssign(lvalue, newValue) compares the two values, updating the lvalue if different, returns BOOL.
- ASLockedSelfCompareAssign(lvalue) Same as above but holds self as lock.
- Other variants of the same things.

…t users/subclasses can access their locks

maicki · 2018-03-24T17:11:15Z

Source/ASNetworkImageNode.mm

  __weak id<ASNetworkImageNodeDelegate> _delegate;

  NSURL *_URL;
  UIImage *_defaultImage;

-  NSUUID *_cacheUUID;
+  std::atomic_uint _cacheToken;


We may should have this change in a follow up diff. Not really 100% related to adopting NSLocking.

maicki · 2018-03-24T17:19:40Z

Source/Private/ASDisplayNode+FrameworkSubclasses.h

@@ -1,40 +0,0 @@
-//
-//  ASDisplayNode+FrameworkSubclasses.h


Nice that we can get rid of that

appleguy

[EDIT] Since investments in our locking infrastructure are relatively rare, it's exciting to see such a thoughtfully considered API design and underlying implementation. Great changes across the board!

Feedback is minor, mostly interested in developing a style recommendation for whether [self lock] vs the AS* methods will be most common and try to standardize on that within the framework. This shouldn't block landing.

It seems like a good idea to adopt NSLocking to encourage third party developers with the need for custom locking to use the -lock/unlock methods.

appleguy · 2018-03-25T01:22:20Z

Source/ASImageNode.mm


  ASImageNodeDrawParameters *drawParameters = [[ASImageNodeDrawParameters alloc] init];
  drawParameters->_image = [self _locked_Image];
-  drawParameters->_bounds = [self threadSafeBounds];
+  drawParameters->_bounds = self.bounds;


Is this particular change necessary, and also safe? IIRC it was important to use threadSafeBounds here.

Oh I didn't know. When I removed the FrameworkSubclasses import, the import for this field was lost and I figured it was OK to use bounds since this method is (allegedly) called on main. I put it back since the change is outside the purview of this diff 👍

appleguy · 2018-03-25T01:25:01Z

Source/ASNetworkImageNode.mm

    __weak id<ASNetworkImageNodeDelegate> delegate = _delegate;
    BOOL delegateDidStartFetchingData = _delegateFlags.delegateDidStartFetchingData;
    BOOL isImageLoaded = _imageLoaded;
    NSURL *URL = _URL;
    id currentDownloadIdentifier = _downloadIdentifier;
-  __instanceLock__.unlock();
+  [self unlock];


The difference in style between these statements and the macros feels a bit less prominent / conspicuous than before, where all locks involved instanceLock.

I wonder if we should use a macro for even the simple lock / unlock message send, just to make locking operations consistent? Even if we did that, lock / unlock would still be available, especially for third party code.

It's true – I'm iffy on this. I totally see your point that there's a mismatch in look and feel, but I don't like when there's two ways to do the same thing, and I like the referential transparency.

I also kind of like the mismatch, since it "discourages" use of these methods by making them stick out.

Let's sleep on this. There will be more locking diffs in the future and we can add the macros easily at any time.

appleguy · 2018-03-25T01:28:22Z

Source/ASVideoNode.mm

-    __instanceLock__.lock();
-
+    ASUnlockScope(self);
+    [self addSubnode:_playerNode];


I'm not sure if it is important to re-lock before the setNeedsLayout, but the locking scope did change here (may want to add a { } scope around the addSubnode)

Good catch, either way it's outside the scope of this diff 👍

appleguy · 2018-03-25T01:30:20Z

Source/Base/ASBaseDefines.h

@@ -232,6 +232,26 @@
  ((c *) ([__val class] == [c class] ? __val : nil));\
 })

+// Compare two primitives, assign if different. Returns whether the assignment happened.
+#define ASCompareAssign(lvalue, newValue) ({  \


Shouldn't there be a variant that does use ASObjectIsEqual, but doesn't do a copy (just a regular assignment which may involve a retain?)

Is there any way to enforce that ASCompareAssign is not used for objects accidentally?

Good point, I added that variant as ASCompareAssignObject.

I think there's value in using plain ASCompareAssign for objects, in the case where we want to skip the isEqual: (say, in setDelegate: or similar), so I'd rather keep that option open.

ghost · 2018-03-25T16:46:33Z

	1 Warning
⚠️	This is a big PR, please consider splitting it up to ease code review.

Generated by 🚫 Danger

* Make display node, layout spec, and style conform to NSLocking so that users/subclasses can access their locks * Update the changelog * Align slashes * Put it back, when we're in ASDisplayNode * Go a little further * Put back the changes I didn't mean to commit * Kick the CI * Fix yoga build * Put back non-locking change * Address comments from Scott

Adlai Holler added 2 commits March 23, 2018 20:39

Make display node, layout spec, and style conform to NSLocking so tha…

737ebea

…t users/subclasses can access their locks

Update the changelog

7ec9d91

Adlai-Holler requested review from maicki, garrettmoon, appleguy and nguyenhuy March 24, 2018 03:50

Adlai Holler added 4 commits March 23, 2018 20:54

Align slashes

49d5693

Put it back, when we're in ASDisplayNode

99aa161

Go a little further

465ff2d

Put back the changes I didn't mean to commit

c5072c8

TextureGroup deleted a comment Mar 24, 2018

Adlai Holler added 2 commits March 23, 2018 22:16

Kick the CI

fc6cd3d

Fix yoga build

da7caf0

TextureGroup deleted a comment Mar 24, 2018

maicki approved these changes Mar 24, 2018

View reviewed changes

Put back non-locking change

454dcd5

appleguy approved these changes Mar 25, 2018

View reviewed changes

Address comments from Scott

9870664

Adlai-Holler merged commit 0f9b1e6 into master Mar 25, 2018

Adlai-Holler deleted the AHNSLocking branch March 25, 2018 17:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make objects conform to NSLocking #851

Make objects conform to NSLocking #851

Adlai-Holler commented Mar 24, 2018

maicki Mar 24, 2018

maicki Mar 24, 2018

appleguy left a comment •

edited

Loading

appleguy Mar 25, 2018

Adlai-Holler Mar 25, 2018

appleguy Mar 25, 2018

Adlai-Holler Mar 25, 2018

appleguy Mar 25, 2018

Adlai-Holler Mar 25, 2018

appleguy Mar 25, 2018

Adlai-Holler Mar 25, 2018

ghost commented Mar 25, 2018

Make objects conform to NSLocking #851

Make objects conform to NSLocking #851

Conversation

Adlai-Holler commented Mar 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

appleguy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Mar 25, 2018

appleguy left a comment •

edited

Loading