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

Allow swift codegen to produce classes instead of structs #17

Open
plivesey opened this issue Jan 25, 2016 · 5 comments
Open

Allow swift codegen to produce classes instead of structs #17

plivesey opened this issue Jan 25, 2016 · 5 comments

Comments

@plivesey
Copy link

I think it's a good idea to allow an option to choose classes for records instead of structs (default). This ticket is a suggest for the future rather than a bug with the current implementation.

First, a couple of things:

  • I work at LinkedIn and have helped on a very similar project to this. We started with structs, but later switched to classes.
  • I am not currently using this library so nothing here is blocking me.

Here's why I think classes are better for this use-case than structs:

  1. Classes are much more stable in swift than structs (compilation). When we used structs (especially with associated types as you do for unions), we uncovered many compiler bugs which were extremely unpredictable and hard to track down. The compiler would usually segfault on some inputs. We filed a bunch of these bugs against Apple and they have fixed some of them, but not all of them.
  2. Classes compile faster. For larger projects, this can become a problem quickly. In our tests, classes compile ~20% faster than structs.
  3. Classes create smaller binaries. Structs can produce much larger binaries. (I think it's something to do with nested associated values getting 'inlined' somehow, but not sure about the details). I forget the exact numbers, but it was significant enough for us to move away from structs for data models.
  4. Classes never need to be copied. I'm not 100% sure on this one, but we did see some hidden performance problems with structs and the compiler 'over-copying' them to be sure. Copy-on-write functionality only seems to be implemented for arrays and dictionaries and you don't get it for custom structs. Though normally this won't be a problem, the additional CPU and memory use seems like a waste. (I don't have any data/checks currently to back this one up...so take with a grain of salt).

I would opt instead to use final class. As far as I can tell, for immutable models, struct and final class are completely interchangeable. As in, from the view of the developer, there is no change in functionality or behavior between the two. Both are immutable, unsubclassable, and thread-safe. (There may be differences under the hood, but none of this will create any changes in program execution).

Therefore, I'd always opt for final class over struct if I could and it'd be cool if this library supported it.

@plivesey
Copy link
Author

Ah, forgot another reason. Not 100% sure if this how much schema support there is for this (but I'm pretty sure PDSC allows it), but:

  1. Structs don't allow recursive definitions. E.g.
struct Recursive {
    let other: Recursive?
}

This could be valid input since other is optional. This could also occur with an associated type. Structs don't allow this, but classes do. I think this also hints at why struct heavy binaries can get really big and compile slower...

@jpbetz
Copy link
Contributor

jpbetz commented Jan 26, 2016

@plivesey,

Thanks for summarizing your learnings. You've clearly thought about this quite a bit!

We've not run into the issues you describe yet (except the Recursive issue, which we have hit), but I expect at some point we would run into the others. The performance impact is particularly worrying, and, honestly, quite disappointing. Typing something as an immutable struct gives the Swift compiler and LLVM strong guarantees for it to leverage when optimizing code.

Re: #4, are you sure? I attended a talk by an apple developer who had contributed to Swift that suggested that the compiler should understand that immutable structs do not require copy (and instead apply techniques similar to c++ move semantics).

But regardless, I think we'll take advantage of your learnings here and transition to final classs.

@plivesey
Copy link
Author

First, for number 4, I am not sure. And if that's the case, then you can ignore that point. That's good to hear, and I might try running some tests. I wonder if they're able to handle computed fields and such (labeled as var), which I assume you're using for default properties? I'd assume so though since there is not mutable memory.

Re: disappointing: Yeah, I've been disappointed by structs in general. They seem to be really useful for small pieces of data that you can pass around, but I don't treat them as the 'default choice' like Apple seems to want us to.

@jpbetz
Copy link
Contributor

jpbetz commented Jan 26, 2016

Not sure if this is exactly what you're asking, but for Pegasus style defaults we do:

class Example {
  let field

  public init(field: String = "default") {
    self.field = field
  }
  // ...
}

But to your point, I would hope structs can handle computed fields and such, but I've not used them at all for mutable data.

Thanks for all the info!

@plivesey
Copy link
Author

Yeah, that makes sense. Not sure what I was thinking of with computed properties...

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

No branches or pull requests

2 participants