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

Add custom operators as alternatives to overloaded operators #93

Merged
merged 10 commits into from
Oct 30, 2020

Conversation

colejd
Copy link
Contributor

@colejd colejd commented Oct 28, 2020

This PR aims to solve the issue of slow type-checking performance due to operator overloading (see #83).

To solve this, I've added new custom operators that provide the same functionality as the overloaded operators. Essentially, users can opt in by surrounding the overloaded operators in a "box of shame" (thanks, Zev):

Operator New Alternative
== |==| (changed to /==/)
<= |<=| (changed to /<=/)
>= |>=| (changed to />=/)

This has a few benefits:

  • By using a custom operator rather than overloading existing operators, we reduce the amount of overhead accrued.
  • The existing operator overloads now fall back on the new operator alternatives, which prevents duplicated code and makes this PR non-breaking.

Here's a build timing summary for an app that makes heavy use of Anchorage:

Build Timing Summary

CompileC (799 tasks) | 1121.137 seconds
CompileSwiftSources (49 tasks) | 723.882 seconds
CompileAssetCatalog (7 tasks) | 329.057 seconds
CompileXIB (3 tasks) | 263.134 seconds
Ld (74 tasks) | 89.242 seconds
CompileStoryboard (4 tasks) | 59.476 seconds
CopyPNGFile (60 tasks) | 51.361 seconds
PhaseScriptExecution (4 tasks) | 10.858 seconds
ValidateEmbeddedBinary (5 tasks) | 8.584 seconds
IntentDefinitionCompile (3 tasks) | 6.932 seconds
Ditto (367 tasks) | 6.898 seconds
CodeSign (6 tasks) | 6.889 seconds
IntentDefinitionCodegen (3 tasks) | 6.850 seconds
CreateUniversalBinary (35 tasks) | 4.496 seconds
Touch (36 tasks) | 4.284 seconds
LinkStoryboards (3 tasks) | 0.042 seconds
Libtool (2 tasks) | 0.021 seconds

Here's the build timing summary after switching to this branch using my new operators:

Build Timing Summary

CompileC (799 tasks) | 1005.547 seconds
CompileSwiftSources (49 tasks) | 682.290 seconds
Ld (74 tasks) | 128.364 seconds
CompileAssetCatalog (7 tasks) | 16.995 seconds
CompileXIB (3 tasks) | 13.545 seconds
ValidateEmbeddedBinary (5 tasks) | 12.560 seconds
CopyPNGFile (60 tasks) | 10.077 seconds
Ditto (367 tasks) | 8.374 seconds
PhaseScriptExecution (4 tasks) | 7.390 seconds
CompileStoryboard (4 tasks) | 7.034 seconds
CreateUniversalBinary (35 tasks) | 5.416 seconds
IntentDefinitionCompile (3 tasks) | 4.970 seconds
IntentDefinitionCodegen (3 tasks) | 4.910 seconds
CodeSign (6 tasks) | 2.332 seconds
Touch (36 tasks) | 0.860 seconds
LinkStoryboards (3 tasks) | 0.024 seconds
Libtool (2 tasks) | 0.021 seconds

Overall, the build time went from 4:55 to 3:22.

@ZevEisenberg
Copy link
Collaborator

It's a hassle, but it would be good to add these to the tests. Or maybe just convert the tests over to the new operators, since they all forward?

Copy link
Contributor

@chrisballinger chrisballinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love it! I'd like to see us take a step further and consider deprecation of the old operator overloads, because their continued usage will always slow down compile times and could lead people toward tech debt without them realizing.

@available(*, deprecated, renamed: "|==|")
@discardableResult public func == (lhs: NSLayoutXAxisAnchor, rhs: LayoutExpression<NSLayoutXAxisAnchor, CGFloat>) -> NSLayoutConstraint {

If we go down the deprecation route, we should do a semver minor version bump, and update the docs/tests to use the new syntax.

@ZevEisenberg
Copy link
Collaborator

Also, in the spirit of the Swift Evolution process, I'd love to see some samples of the new operators in action, so we can see how it actually looks, rather than having to imagine it. If we go @chrisballinger's route, we'll be able to see it in the sample app.

@colejd
Copy link
Contributor Author

colejd commented Oct 29, 2020

@ZevEisenberg should I just replace the overloaded operators with the new ones in the sample app, or do you think it would be better to provide some alternative examples? I guess the answer would really depend on if we want to deprecate the overloaded operators, though.

@colejd
Copy link
Contributor Author

colejd commented Oct 29, 2020

Would love to get @jvisenti's opinion on all of this, too.

@jvisenti
Copy link
Contributor

Changing to custom operators is something that's been discussed for a long time. I think it's good they're finally implemented, even though I've had reservations in the past. I'm not so sure about deprecating the old operators though, because Anchorage has always been about convenience and typing |==| is less convenient than typing == (I also find it a bit less readable). I personally would use a fork or earlier version if the old operators were deprecated, but I'm also not optimizing how I write code for reduced compile times.

@colejd
Copy link
Contributor Author

colejd commented Oct 29, 2020

This is kind of how I felt about it too. I think the tech debt isn't a significant issue if your compile times are reasonable. If you start getting warnings that your functions are taking a very long time to compile, the new operators provide you a way out (just put the operators you already wrote "in jail").

You're also right that it's a pain to type. If you have any ideas for a better syntax I'm all ears. Maybe brackets would be easier to type out than pipes (e.g. [==]?)

@colejd
Copy link
Contributor Author

colejd commented Oct 29, 2020

Ah, brackets aren't allowed in operator names. I guess the real goal is to reduce the amount of cognitive overhead it takes to type out the operators, and my thought was to reduce the need to alternate pressing/releasing the Shift key a lot.

I tried typing out a few alternatives to find something that's less difficult. Is /==/ too weird?

@colejd
Copy link
Contributor Author

colejd commented Oct 29, 2020

@ZevEisenberg The overloaded operators are internally leaning on the new operators. I'm hesitant to update the tests with the new operators, as we could run into a future situation where a regression is introduced for the overloaded operators and our tests wouldn't catch it.

This reverts commit 8ecebd5.

# Conflicts:
#	AnchorageTests/AnchorageTests.swift
Source/Anchorage.swift Outdated Show resolved Hide resolved
Source/Anchorage.swift Outdated Show resolved Hide resolved
@ZevEisenberg
Copy link
Collaborator

On the topic of testing, I defer to the old adage, "the only code you need to test is code you want to work." Test both? Too much work to code-gen the tests for both operators? It's not like this is an unbounded problem. You're probably five minutes of find-and-replace away from doubling up the tests.

@colejd
Copy link
Contributor Author

colejd commented Oct 29, 2020

I've updated the tests to cover overloads and custom operators independently. I was worried that making the overload invoke the custom operator would hurt runtime performance (however negligibly), so I wrote some performance tests as well. Turns out the custom operators are about 4% faster.

Remove views after each test to balance their addition before each test
@colejd
Copy link
Contributor Author

colejd commented Oct 29, 2020

I think this is ready for another round of review (@ZevEisenberg @chrisballinger @jvisenti). To summarize:

  • The operators are now surrounded with / instead of | to make them easier to type.
  • Documentation has been added to the README that includes an example.
  • Tests have been updated to cover operator overloads and custom operators. I've also added performance testing for both sets of operators.
  • The Cocoapods version has been bumped from 4.4.0 to 4.5.0.
  • For now, we're preserving the operator overloads, and the new custom operators are intended as a fix if a user encounters long compile times. The discussion for deprecating the overloads and reworking the documentation should be done in another issue or PR, in my opinion.

Copy link
Contributor

@chrisballinger chrisballinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with all of your thoughts here. I do think that |==| visually looks better than /==/ but it's definitely harder to type.

@ZevEisenberg
Copy link
Collaborator

It's just italic! 😂

I agree with everything here as well. Nice work, @colejd

@ZevEisenberg
Copy link
Collaborator

Although I'd usually save the version bump for a separate PR. Separation of concerns 😉

@colejd colejd merged commit 23a6dd0 into master Oct 30, 2020
@colejd colejd deleted the feature/colejd/add-custom-operators branch October 30, 2020 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants