Skip to content

[Observation] Optimize the storage of registrar entries, provide KeyPath caching, and distinctness notification #78151

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

Merged

Conversation

phausler
Copy link
Contributor

This alters the macro code-generation for Observable types. The types now house a distinctness check by testing shouldNotifyObservers which by default emits only on new values for things that conform to equatable.

Furthermore it removes the bottleneck of creating key paths by pre-caching them as cachedKeypath prefixed static members. This avoids the cache miss potential of KeyPaths and also avoids the cache lookup/lock acquisition (which dominate most of the traces of high frequency accessed properties for observable types).

Additionally some table bookkeeping was removed so that the registrar itself is now faster.

@phausler
Copy link
Contributor Author

@swift-ci please smoke test

@phausler phausler merged commit 8462f5c into swiftlang:main Dec 16, 2024
3 checks passed
@vanvoorden
Copy link
Contributor

I think it's awesome that we're shipping some performance improvements to the way we use key paths and our registrar implementations… but I'm a little unclear about the decision to ship the distinctness checks. I'm not able to find any mention of this discussed on the Swift Forums. Was there any public discussion about how this would affect engineers building against Observable?

I understand we want to help product engineers write efficient code and try to control for the data that goes in SwiftUI… but AFAIK this changes the semantics of what an Observable type is and how it behaves in non-trivial ways.

Here is one example. Suppose we define a class data model:

class Car {
  var name: String = ""
  var needsRepairs: Bool = false
  
  init(name: String, needsRepairs: Bool = false) {
    self.name = name
    self.needsRepairs = needsRepairs
  }
}

extension Car: Equatable {
  static func == (lhs: Car, rhs: Car) -> Bool {
    if lhs === rhs {
      return true
    }
    guard
      lhs.name == rhs.name
    else {
      return false
    }
    guard
      lhs.needsRepairs == rhs.needsRepairs
    else {
      return false
    }
    return true
  }
}

This is a reference type (AnyObject) that is also Equatable.

We now define a Box with Observable:

@Observable class Box<T: Equatable> {
  var value: T?
}

let box = Box<Car>()
let c1 = Car(name: "1234")
box.value = c1
let c2 = Car(name: "1234")
box.value = c2

precondition(box.value === c2)

That code runs without any problems building from 6.0… but crashes from 6.2. The semantics of setting variables (not just the semantics of when change notifications are dispatched) has changed. This code looks kind of weird… we just set box.value = c2… so why is box.value !== c2? Would the plan be for the documentation to be expanded for engineers to learn why this happened?

Was there any discussion about continuing to set variables eagerly… but only firing the change events when the distinctness checks say to? As opposed to using distinctness checks to tell us whether or not to even set the variable?

The other question is to what extent can engineers "opt out" of this new behavior in a way that is stable and (to the best extent possible) also "future proof".

Currently the shouldNotifyObservers family seem to leverage static dispatch to choose the exact function to call. Would this be safe to assume that there is no reason for engineers to assume this would ever migrate to a dynamic dispatch in the future?

If we can assume these will be chosen statically at compile time… this seems to be one legit workaround to opt out of this new behavior:

@Observable class Box<T> {
  var value: T?
}

Because the compiler has no knowledge this T is Equatable… we can assume this will get us the "legacy" behavior of Observable (with no distinctness checks)?

If we do need to tell the compiler that this type is Equatable (or AnyObject)… this seems to be another workaround:

@Observable class Box<T: Equatable> {
  var value: T?
  
  private nonisolated func shouldNotifyObservers<Generic>(_ lhs: Generic, _ rhs: Generic) -> Bool {
    return true
  }

  private nonisolated func shouldNotifyObservers<Generic: Equatable>(_ lhs: Generic, _ rhs: Generic) -> Bool {
    return true
  }

  private nonisolated func shouldNotifyObservers<Generic: AnyObject>(_ lhs: Generic, _ rhs: Generic) -> Bool {
    return true
  }

  private nonisolated func shouldNotifyObservers<Generic: Equatable & AnyObject>(_ lhs: Generic, _ rhs: Generic) -> Bool {
    return true
  }
}

This seems to work… but AFAIK the shouldNotifyObservers are "the same" across all observed variables. In this example we only observe one variable… so this opts out of distinctness checks for one variable. If our Box defined multiple variables that were all ObservationTracked I don't see any additional context here that would help us opt out of "just one" variable if more than one variable can call into the shouldNotifyObservers.

Was there any discussions about a public API for engineers to opt out of distinctness checks? Are any of these workarounds planned to ship as the official or recommended workaround?

I'm not sure there is "one right" answer to any of these questions… there are tradeoffs and pros and cons involved. Could the community help with any more input and involvement in what some potential next steps might look like for Observable?

@bdkjones
Copy link

@vanvoorden Thanks for the thorough examples and for raising concerns. I've built an entire ORM that is deeply intertwined with Observation and would have loved a heads-up that these changes were coming.

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