Performance and behaviour improvements

[andrebraga]

Test the observation state of the hidden flag instead of always performing all the work for no reason. Brings an immense (8x faster on a particular project) performance boost.

Use constraint activation APIs on iOS8+, which should be more performing. In tandem, use a (weak objects) NSHashTable instead of an array of NSLayoutConstraints because constraints make unsafe_unretained/assign/unowned(unsafe) references to the views, which might be deallocated (and were, in that particular situation). This allows constraints to be deallocated by ARC when there are no strong references left (using simple arrays led to crashes while the layout engine walks the view hierarchy when constraints are de/activated).

Employ the predicate negation function, should allow slightly better code generation by not creating extra closures.

[CosynPa]

Some tests don't pass

[andrebraga]

I'll take a look. There should be no functional changes...

[andrebraga]

I'll take a look soon as I can. The performance gains are too substantial to ignore.

[andrebraga]

This should do it. However the same-constants tests are failing, but I believe the reason is benign: the iOS 8+ API for constraint activation is installing them on the most optimized places.

[CosynPa]

Isn't the activation API installing a constraint on the nearest common ancestor?

[andrebraga]

I believe so; did you find another issue?

[CosynPa]

I'll check the failing test. This test is very important, even more important than all the other tests. It's very strange, if Apple developers who implemented UIStackView were also using the activation API, there should be no difference.

[andrebraga]

Hi. I don't think they use this API. It is quite probable that UIStackView has been under development for a couple years before it was introduced, there is a big chance they used legacy APIs themselves. The other probable cause is that the activation API could lead to the crashes I experienced as views are deallocated while the constraints are still installed somewhere. The deactivation does try to reach for the views for some reason and as pointed out, the references are unsafe.

And for all I know I could have introduced bugs. The code does seem sound, but...

I'm clueless though about the tests that fail for other reasons, likely the use of NSHashTable allows for early deallocations. I'd love to cooperate in the investigation but again time is short. I'm all ears for pointers and tips.

[CosynPa]

But they are also refactoring their code. I noticed a minor change that adjusted the order of constraints in an iOS update, which reflected that they possibly changed to use same code for both horizontal and vertical stack view.

[CosynPa]

I see the problem. In UIStackView they use some objects of type UILayoutGuide, which is introduced in iOS 9, to help layout. And In TZStackView, we use empty UIViews for the same purpose. A UILayoutGuide can't add any constraint to itself, so the constraints are added to its owning view. But in TZStackView some constraints are added to the guide view itself when using activation API. That's why some constraints are not installed in the same view as UIStackView.

[andrebraga]

Oh, nice job finding the issue. So there's no harm in the end? The activation API should always produce equal or better results. What do you think? Perhaps adding a test to ensure the diverging results fall into this category?

[CosynPa]

A tricky solution will be using UILayoutGuide when iOS 9 is available, that way we don't need to modify the test method.

[andrebraga]

Hey. Any news on this front? 😃

[CosynPa]

Do you have any code that can reproduce the crash you've mentioned? The pattern where store constraints in an array is quite common, it's hard to believe this pattern has such serial issue.

[andrebraga]

It is caused by the other changes, actually. Moving to the activation API led to situations where it could crash. I mentioned it in a comment circa a month ago: https://github.com/CosynPa/TZStackView/pull/3#issuecomment-228942980

[CosynPa]

When a view is removed from superview, its related constraints will be automatically deactivated. Then deactivating an already deactivated constraint should have no effect, even if its firstItem or secondItem is a dangling pointer.

[andrebraga]

Unfortunately this is not what I am observing. You can see it for yourself. Just revert the NSHashTable-related changes and you'll face crashes if there are views being removed from the hierarchy. I'll see if I can come up with a reduced test case but trust me, this is happening indeed.

[CosynPa]

At least the statement "when a view is removed from superview, its related constraints will be automatically deactivated" is true, and is documented in UIView class reference removeFromSuperview method:

Calling this method removes any constraints that refer to the view you are removing, or that refer to any view in the subtree of the view you are removing.

I did face some strange layout engine internal error crashes. But using hash table didn't fix them.

[andrebraga]

I think I should probably split the PRs and for the time being only incorporate the one related to not producing constraints when not in a window.

[CosynPa]

How are your PRs? I found there is code accessing firstItem:

private func constraintContains(view: UIView) -> (NSLayoutConstraint) -> Bool {
    return { ($0.firstItem as? UIView) == view || ($0.secondItem as? UIView) == view }
}

Maybe this is the source of crash?

[CosynPa]

I found a relative issue https://github.com/SnapKit/SnapKit/issues/277. I don't know if your crash also happens when accessing firstItem or secondItem.