Skip to content
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

Adds custom operators for speed #293

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

acrookston
Copy link

@acrookston acrookston commented Jun 14, 2018

Addresses #215.

This is maybe not the best solution (not DRY) but with the code below we've seen swift type checking times go down from 8800ms+ to 39ms (example at bottom).

With this PR I wanted to start a discussion around using custom operators. As I understand it, the problem is the overloading of common operators like ==, - and + causing confusion for the type checker and massive compile times.

Other than tests, not sure what is missing but I'd be happy to make any improvements.

Example code

A fairly complex example, here seen with the updated syntax.

constrain([self, titleLabel, dialCodeButton, phoneTextField, submitButton]) { proxies in
            var views = proxies
            let superview = views.removeFirst()
            let titleLabel = views.removeFirst()
            let dialCodeButton = views.removeFirst()
            let phoneTextField = views.removeFirst()
            let submitButton = views.removeFirst()

            for view in [titleLabel, submitButton] {
                view.left ~== superview.left ~+ GUI.Constant.tableInset
                view.right ~== superview.right ~- GUI.Constant.tableInset
            }

            dialCodeButton.left ~== superview.left ~+ GUI.Constant.tableInset
            phoneTextField.left ~== dialCodeButton.right ~+ padding
            phoneTextField.right ~== superview.right ~- GUI.Constant.tableInset

            dialCodeButton.width ~== (superview.width ~- (GUI.Constant.tableInset * 2)) ~* 0.2
            phoneTextField.width ~== (superview.width ~- (GUI.Constant.tableInset * 2)) ~* 0.8

            submitButton.height ~== GUI.Constant.buttonHeight
            phoneTextField.height ~== GUI.Constant.buttonHeight
            dialCodeButton.height ~== GUI.Constant.buttonHeight

            titleLabel.top ~== (superview.top + GUI.Constant.tableInset)
            phoneTextField.top ~== (titleLabel.bottom + GUI.Constant.tableInset)
            dialCodeButton.top ~== (titleLabel.bottom + GUI.Constant.tableInset)
            submitButton.top ~== (phoneTextField.bottom + padding)
        }

@acrookston
Copy link
Author

Another example, much simpler takes 3600ms.

constrain(self, textLabel, helpButton) { superview, textLabel, helpButton in
            let textWidth = superview.width - iconWidth - padding - GUI.Constant.tableInset * 2
            textLabel.left == superview.left + GUI.Constant.tableInset
            helpButton.width == iconWidth
            textLabel.width == textWidth
            helpButton.right == superview.right - GUI.Constant.tableInset
            textLabel.centerY == superview.centerY
            helpButton.centerY == superview.centerY
            textLabel.height == GUI.Constant.rowHeight
            helpButton.height == GUI.Constant.rowHeight
        }

@acrookston
Copy link
Author

After migrating most of the code in one of our apps over to the new syntax the build time has been halved (from about 5 min to 2.5). All of our view constraints all run in a few milliseconds where most were in the hundreds, if not thousands. Strongly recommend the core team looks over this issue and applies some (maybe not my) solution. Thanks for a great library!

@LucasVanDongen
Copy link

LucasVanDongen commented Jun 15, 2018

Always felt it was due to something like this. It's a pity. But the == operator is a lie anyway as with a true == it doesn't matter what you put left or right and that clearly isn't the case with Cartography.

@orta
Copy link
Collaborator

orta commented Jun 16, 2018

Is it this a semver break?

@acrookston
Copy link
Author

@orta technically no, but it's adding to the API and suggesting a new API so "perhaps"? With the current iteration I'm not replacing the current operators, I've only copied or "aliased" them (hence non-DRY). Was hoping to spark a discussion on how this would be best implemented upstream. I'm willing to take lead on any required changes.

@orta
Copy link
Collaborator

orta commented Jun 16, 2018

I'm basically the person running this repo, but I've never used it, nor really grok it's trade-offs as I've been absent from Swift for enough years.

So I'm happy to go with consensus, and those compilation speed improvements make any pain worth it IMO - it's bad enough without a library making it worse. Did tests pass for you locally? Do we need to update travis?

@fssilva
Copy link

fssilva commented Dec 20, 2018

Is there an ETA for this fix to be merge ?

@orta
Copy link
Collaborator

orta commented May 10, 2019

No ETA, maybe a rebase might fix CI though

n3d1117 pushed a commit to n3d1117/appdb that referenced this pull request May 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants