Swiftgen: Asset Catalog: default template is very CPU intensive on lookup

[eaigner]

Assume I have a color named myColor. Accessing Asset.myColor.color from the generated assets swift file is very expensive, since this will always look up the color and is not lazily evaluated.

I profiled it and if the color is accessed frequently this incurs a heavy performance penalty.

[AliSoftware]

Interesting, thanks for the feedback!

Would changing those lines in the template to this below help?

  {{accessModifier}} private(set) lazy var color: {{colorAlias}} = {{colorAlias}}(asset: self)

I'd love if you could use the same thing you used to profile the current default template to profile a custom template with this change to confirm it would solve it

[eaigner]

Can't unfortunately because the struct then seems to want an initializer for the lazy property as well and breaks other stuff.

[AliSoftware]

Ah I see. And if we provide a dedicated init ourselves, then there's the matter that lazy var color has to be mutating anyway…

---

The other obvious solution is to make it a let, which honestly would probably make sense. One might think that this would lead to all colors being evaluated at startup for nothing, but the fact that the ColorAsset constants themselves generated in the nested enums are globals (Asset.Colors.Vengo.primary for example like in our unit tests), and global let are automatically lazy (in fact if I try to change it to lazy var primary = …, the compiler tells us "'lazy' must not be used on an already-lazy global"), so this should be a nice solution…

But 😒 then we can't pass self as a parameter when setting let color = ColorAsset(asset: self) now that it's a let and not a computed property nor a lazy var… and additionally, it seems like @available can't be used on stored property anyway (at least that's what a playground yells at me)…

If anyone have a suggestion of what construct we can use as a solution, I'm interested.

[AliSoftware]

Ah, maybe instead of having struct ColorAsset then an extension UIColor to add init(asset:) to it, we could feed both the name and color properties in the ColorAsset.init(name:). Given that I just realised that the constants we declare at top-level (e.g. Asset.Colors.Vengo.primary = ColorAsset(name: "Vengo/Primary")) are already lazy, this shouldn't be a problem after all…

So this would become this instead:

@available(iOS 11.0, tvOS 11.0, watchOS 4.0, OSX 10.13, *)
{{accessModifier}} struct {{colorType}} {
  {{accessModifier}} let name: String
  {{accessModifier}} let color: {{colorAlias}}
  init(name: String) {
    self.name = name
    let bundle = Bundle(for: BundleToken.self)
    #if os(iOS) || os(tvOS)
    self.color = {{colorAlias}}(named: name, in: bundle, compatibleWith: nil)
    #elseif os(OSX)
    self.color = {{colorAlias}}(named: NSColor.Name(name), bundle: bundle)
    #elseif os(watchOS)
    self.color = {{colorAlias}}(named: name)
    #endif
  }
}

Which will be equivalent to this on iOS:

@available(iOS 11.0, *)
internal struct ColorAsset {
  internal let name: String
  internal let color: UIColor
  init(name: String) {
    self.name = name
    let bundle = Bundle(for: BundleToken.self)
    self.color = UIColor(named: self.name, in: bundle, compatibleWith: nil)!
  }
}

Thoughts? Do you think that will solve the performance issue?

[djbe]

It's weird that Apple didn't use some caching mechanism as they did with UIImage's.

@AliSoftware Seems like a good solution, and easy enough to implement.
@eaigner Can you confirm if this solves your issue?

[eaigner]

Seems to work, however the initializers require an explicit unwrap in the stencil

@available(iOS 11.0, tvOS 11.0, watchOS 4.0, OSX 10.13, *)
{{accessModifier}} struct {{colorType}} {
  {{accessModifier}} let name: String
  {{accessModifier}} let color: {{colorAlias}}
  init(name: String) {
    self.name = name
    let bundle = Bundle(for: BundleToken.self)
    #if os(iOS) || os(tvOS)
    self.color = {{colorAlias}}(named: name, in: bundle, compatibleWith: nil)!
    #elseif os(OSX)
    self.color = {{colorAlias}}(named: NSColor.Name(name), bundle: bundle)!
    #elseif os(watchOS)
    self.color = {{colorAlias}}(named: name)!
    #endif
  }
}

[AliSoftware]

Ah cool, yeah indeed forgot the force unwrap. Usually I'm all against using force unwrap in code, which is also why I created SwiftGen to create safe code, but for code generated from asset catalogs so guaranteed to exist, it's fine by me.

We could use a guard let … else { fatal error("some nice error message") } instead of the force unwrap (like we do in other templates iirc?) instead just to make it nicer but otherwise I'm good with it.

Btw I assume the performance issue has disappeared then with this new code right?
If so, we'd love a PR with this change. Just make the change on all templates, then run the "Generate Outputs" scheme to automatically update the unit tests accordingly, and credit yourself in the CHANGELOG 👌

[djbe]

We can probably have a local optional, fill it with the right color init (platform dependent), and then guard let to assign it to self.color

[djbe]

QQ: why not use final class with lazy var?

The thing is we want to keep the @available(...) for the color property, otherwise we have to move that availability annotation up to the static let asset declaration, which would lead to a lot of extra annotations for all users.

[djbe]

Implemented in #589.