Rightpoint / Anchorage

A collection of operators and utilities that simplify iOS layout code.
MIT License
627 stars 46 forks source link

Add custom operators as alternatives to overloaded operators #93

Closed colejd closed 3 years ago

colejd commented 3 years ago

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:


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 commented 3 years ago

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?

ZevEisenberg commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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

jvisenti commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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.

ZevEisenberg commented 3 years ago

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 commented 3 years ago

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.

colejd commented 3 years ago

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

ZevEisenberg commented 3 years ago

It's just italic! 😂

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

ZevEisenberg commented 3 years ago

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