Document why Widget.operator== is marked as non-virtual

[rrousselGit]

Why is Widget.operator== marked as non-virtual?

Trying to override == on a widget triggers a warning, but there's no explanation on why (neither in the warning nor in the sources), and is not immediately obvious as to why we shouldn't do so.

[dnfield]

We should probably document this.

I'm not sure why you'd want to override it though. Widgets are expected to be immutable. Even though Dart can't really guarantee immutability of e.g. a child list, the framework breaks if you mutate a property of the widget.

The default implementations of equals (i.e. identical) and hashCode (i.e. identityHashCode) should be good enough for that, and implementing it otherwise could slow things down when the framework goes to compare widgets in the tree.

/cc @Hixie @goderbauer who probably have more insight into this.

[rrousselGit]

Flutter optimize rebuilds around the == operator.

Overriding == allows filtering rebuilds in situations that would otherwise rebuild for no reason.

It's mostly for leaf nodes though.

[dnfield]

Can you give an example?

[rrousselGit]

class Leaf extends StatelessWidget {
  const Leaf({Key key, this.label}) : super(key: key);

  final String label;

  @override
  bool operator ==(Object other) {
    return other is Leaf && other.key == key && other.label == label;
  }

  @override
  int get hashCode => hashValues(key, label);

  @override
  Widget build(BuildContext context) {
    return Text(label);
  }
}

This makes the widget behaves like a const instance (don't rebuild if didn't change), but works with dynamic values.

[goderbauer]

Some context for why this was done is in the linked from the PR that added the @nonVirtual annotation: #46900

[dnfield]

@goderbauer - I did look through that and saw a few places where @Hixie said not to do it, but never why.

[rrousselGit]

It was likely about non-leaf widgets.

If a widget has a child, and that child too overrides ==, then we could end up effectively comparing the entire widget tree.

But I don't see any issue with leaf widgets though.

[Hixie]

Do we have any benchmarks for the leaf case? I wouldn't be surprised if the mere existence of that operator== made everything more expensive (since it turns a single-dispatch into a dynamic dispatch for every single widget comparison ever).

[jonahwilliams]

I think if we don't want developers to implement Widget.==, we should just use identical instead

[dnfield]

That could be confusing - someone expects the framework to be using their overridden ==, but it's actually just using identical and returning an unexpected result...

[jonahwilliams]

Regardless of what choice we make here (keeping nonVirtual, using identity) we won't be able to get away with documenting it extensively. equality as a concept is too heavily overloaded

[jonahwilliams]

I meant "without" documenting it extensively

[t-artikov]

I am pretty sure there are cases when Widget.operator== overriding is useful.
Consider an example: https://dartpad.dev/c20abd67d2cde74263ba1b49b368e062
There is a dynamic list of Items displayed in ListView. Overriding the operator== allows to avoid rebuilding all ItemViews in the viewport on Item addition, only a new one is built (or none if it happens outside the viewport).
This reduces worst frame build time from 25ms to less than 16ms on my device in profiling mode.

[yjbanov]

If operator== is marked as non-virtual, then it shouldn't be confusing for us to switch from == to identical in the framework any more, and the latter should give the compiler a much more direct route to generate the fastest possible native code (e.g. it no longer needs to do whole-world analysis to figure out that the call to == boils down to identical anyway). FWIW, AngularDart has been using identical for this same use-case and saw big performance gains at the time (as much as dart2js can be compared to Dart AOT, of course).

[goderbauer]

Performance comparison between operator== and identical when used inside Element.updateChild. These results were obtained by running the stock_build_iteration microbenchmark on a Moto G4. Seems like identical has a very slight edge.

[yjbanov]

@goderbauer There are two things that can affect performance: the performance of the comparison itself, and the performance of the framework due to changes in comparison semantics. The former is probably relatively small. The latter can be much bigger depending on the app. For example, the comparison can shut off the rebuilding of an entire subtree of an app if the comparison returns true at the root widget of the subtree (i.e. it never calls child.update(newWidget);). I'm not sure if stock_build_iteration exercises that part of the code. I always thought that benchmark was rebuilding everything from scratch on every frame.

[yjbanov]

To be clear, I'm in favor of using the identical semantics because operator== semantics is hard to get right, but also because Flutter makes it trivial localize updates by allowing you to separate fast changing configuration from slowly changing configuration. For example, instead of having an expensive build() function that largely produces "equal" subtrees of widgets you can isolate the changing parameter and push it down into one of RenderObjectWidgets (e.g. via an AnimatedBuilder).

[t-artikov]

@yjbanov
Could you show how my example #49490 (comment) can be optimized if operator== overloading is not available?

[rrousselGit]

@t-artikov With the == out of the way, we still have the ability to "cache" the widget instance.

Here's a quick example:

class Cache<T> extends StatefulWidget {
  const Cache({Key key, this.builder, this.value}) : super(key: key);

  final Widget Function(BuildContext context, T value) builder;
  final T value;

  @override
  _CacheState<T> createState() => _CacheState<T>();
}

class _CacheState<T> extends State<Cache<T>> {
  Widget cache;
  T previousValue;

  @override
  Widget build(BuildContext context) {
    if (widget.value != previousValue) {
      previousValue = widget.value;
      cache = Builder(
        builder: (context) => widget.builder(context, widget.value),
      );
    }
    return cache;
  }
}

Then used this way:

Cache<int>(
  value: 42,
  builder: (context, value) {
    return Text('$value');
  },
);

Where builder is called again only if value (or an InheritedWidget) changes.

It is less ideal for multiple reasons though.

[t-artikov]

@rrousselGit
Thanks!

It is less ideal for multiple reasons though.

I agree, in my opinion, it:

• Less intuitive;
• Has less applicability (doesn't work with multiple parameters);
• Probably, less performant, because creates additional objects;
• Non standard (Flutter doesn't have such widget now)

[dnfield]

Aren't keys helpful here?

[goderbauer]

I was actually just in the process of running some more benchmarks. I modified the stocks app slightly (source) to make sure it can benefit from overriding operator== in one of the Widgets that doesn't take any children. The table below shows the result of running build_bench.dart on that modified Stocks app. The first column "normal" is without adding the operator== override. The second column "override" overrides operator== for that one widget, the third column runs a modified version that uses a cached widget (source).

It's pretty clear that overriding operator== gives you a performance advantage over not doing it ("normal" vs "overridden"). However, you can achieve the same result by caching (see third column).

Overriding operator== is not without flaws. I would consider it error-prone, it only actually works in very limited use cases, and it's easy to mess up and accidentally walk the entire widget hierarchy in your operator==, which would make performance actually worse.

The cache approach seems more flutter-like ("everything is a widget") and you get the same performance. Developers are also already familiar with all concepts involved here and don't have to learn anything new.

In my opinion, we should leave things as they are and repurpose this bug as an issue to document what you should do instead if you think you want to override operator==.

[rrousselGit]

Overriding operator== is not without flaws. I would consider it error-prone

But it can be done automatically.
Code generators can do it for you, and Dart may even include data classes at some point.

Caching, on the other hand, is very tedious to write and cannot be automatized.

[rrousselGit]

Also, by using cache instead of ==, we are reverting from declarative programming to imperative programming, and have to manage a state.

The resulting code is a lot worse than what == does, in my opinion that is.

[goderbauer]

Caching, on the other hand, is very tedious to write and cannot be automatized.

I believe you could also write a code generator that generates this general caching pattern shown in my change: goderbauer@8ebb668

[christian-muertz]

The resulting code is a lot worse than what == does

I agree with that. Implementing the caching pattern requires a stateful widget and some more work. Overriding == is way simpler.

I would consider it error-prone

It should be definitely documented that overriding == has to be done carefully to avoid comparing whole subtrees. It should only be recommended for widgets that do not take children.

[nioncode]

Aren't keys helpful here?

@dnfield I'm actually surprised that adding keys in a ListView.builder doesn't seem to change anything. Could somebody please explain why in the following code all items rebuild whenever a new one is added although they each have a key? Is it therefore recommended to add a cache for the items in a builder to prevent rebuilding?

import 'package:flutter/material.dart';

void main() {
  final app = MaterialApp(
    home: MyHomePage(),
  );
  runApp(app);
}

class Item {
  const Item(this.title, this.subtitle);

  final String title;
  final String subtitle;

  static Item createByIndex(int index) {
    return Item('Item $index', 'Subtitle $index');
  }
}

class MyHomePage extends StatefulWidget {
  @override
  _MyHomePageState createState() => _MyHomePageState();
}

class _MyHomePageState extends State<MyHomePage> {
  final items = List.generate(5, Item.createByIndex);

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      backgroundColor: Colors.white,
      appBar: AppBar(
        title: Text('Rebuild Optimization'),
      ),
      body: ListView.builder(
        itemCount: items.length,
        itemBuilder: (context, index) {
          return ItemView(items[index], ValueKey(index));
        },
      ),
      floatingActionButton: FloatingActionButton(
        child: Icon(Icons.add),
        onPressed: () {
          setState(() {
            items.add(Item.createByIndex(items.length));
          });
        },
      ),
    );
  }
}

class ItemView extends StatelessWidget {
  const ItemView(this.item, Key key) : super(key: key);

  final Item item;

  @override
  Widget build(BuildContext context) {
    print('build ${item.title}');
    return ListTile(
      title: Text(item.title),
      subtitle: Text(item.subtitle),
    );
  }
}

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.