Fix wasted memory caused by debug fields - 16 bytes per object (when adding that should-be-removed field crosses double-word alignment)

[fzyzcjy]

Close #113940

Theoretical analysis

Consider the following example code. Code with "case A" occupies 2.5GB memory, while "case B" is 4.0GB memory.

class C {
  int? a;
  int? b;

  // int get computed => a.hashCode; // case A
  int get computed => a.hashCode ^ b.hashCode; // case B
}

Future<void> main() async {
  final arr = <C>[];
  for (var i = 0; i < 100000000; ++i) {
    arr.add(C()
      ..a = 42
      ..b = 100);
  }
  print('hash=${Object.hashAll(arr.map((e) => e.computed))}');

  print('sleep...');
  await Future<void>.delayed(const Duration(seconds: 10000));
}

Therefore, we know that, a field does not occupy memory if there is no read/write to it. For example, the field b in case A is never read, so Dart compiler seems so smart that it knows it can be eliminated and no memory is allocated for that. By the way, b is written even in case A, but seems memory is not allocated as long as it is not read. This agrees with common sense (but since I am not compiler expert, feel free to correct me if I am wrong!).

For fields inside Flutter that is merely used for debug, they are usually accessed inside an assert(() { ... }()); block. That is great, because if each and every field access are inside assert, those code will be eliminated in release build, and by discussions above, we will not pay memory for those debug fields.

However, AnimationController.debugLabel does not seem to follow this. Inside AnimationController.toStringDetails, it uses debugLabel field without being inside a assert block. Therefore, IMHO, we will be paying the memory of debugLabel even if we never use that in runtime.

As for why toStringDetails is guaranteed to be called, it is simple: Animation.toString calls that toStringDetails, and we know toString will not be tree shaken out.

Experimental analysis

Setup

Use these code:

Need to add two dummy fields to AnimationController, such that it crosses the double-word alignment. (Yes, this PR does not affect memory for today's AnimationController, but who can guarantee it never have two more fields or two less fields, which this PR will reduce 16 bytes).

class AnimationController {
  int? dummy1;
  int? dummy2;
... old code ...
}

Use this code

import 'package:flutter/material.dart';
import 'package:flutter/scheduler.dart';

void main() => runApp(const MyApp());

class MyApp extends StatefulWidget {
  const MyApp({Key? key}) : super(key: key);

  @override
  State<MyApp> createState() => _MyAppState();
}

class _MyAppState extends State<MyApp> {
  final arr = <AnimationController>[];

  @override
  void initState() {
    super.initState();
    for (var i = 0; i < 2000000; ++i) arr.add(AnimationController(vsync: const TestVSync()));
    print('${arr.first.dummy1} ${arr.first.dummy2}'); // ensure the dummy fields are not removed
    arr.first
      ..duration = const Duration(seconds: 1)
      ..forward();
  }

  @override
  Widget build(BuildContext context) => Container();
}

class TestVSync implements TickerProvider {
  const TestVSync();

  @override
  Ticker createTicker(TickerCallback onTick) => Ticker(onTick);
}

Operations

1. flutter build apk --extra-gen-snapshot-options='--print-object-layout-to=object_layout.json' and look at json to know memory layout
2. flutter run --profile to know memory consumption at runtime

Results

Without PR

96 bytes per object

  {
    "class": "AnimationController",
    "size": 96,
    "fields": [
      {
        "field": "dummy1",
        "offset": 20
      },
      {
        "field": "dummy2",
        "offset": 24
      },
      {
        "field": "lowerBound",
        "offset": 28
      },
      {
        "field": "upperBound",
        "offset": 36
      },
      {
        "field": "debugLabel",
        "offset": 44
      },
      {
        "field": "animationBehavior",
        "offset": 48
      },
      {
        "field": "duration",
        "offset": 52
      },
      {
        "field": "reverseDuration",
        "offset": 56
      },
      {
        "field": "_ticker",
        "offset": 60
      },
      {
        "field": "_simulation",
        "offset": 64
      },
      {
        "field": "_value",
        "offset": 68
      },
      {
        "field": "_direction",
        "offset": 72
      },
      {
        "field": "_status",
        "offset": 76
      },
      {
        "field": "_lastReportedStatus",
        "offset": 80
      }
    ]
  },

and

With this PR

80 bytes per object

  {
    "class": "AnimationController",
    "size": 80,
    "fields": [
      {
        "field": "dummy1",
        "offset": 20
      },
      {
        "field": "dummy2",
        "offset": 24
      },
      {
        "field": "lowerBound",
        "offset": 28
      },
      {
        "field": "upperBound",
        "offset": 36
      },
      {
        "field": "animationBehavior",
        "offset": 44
      },
      {
        "field": "duration",
        "offset": 48
      },
      {
        "field": "reverseDuration",
        "offset": 52
      },
      {
        "field": "_ticker",
        "offset": 56
      },
      {
        "field": "_simulation",
        "offset": 60
      },
      {
        "field": "_value",
        "offset": 64
      },
      {
        "field": "_direction",
        "offset": 68
      },
      {
        "field": "_status",
        "offset": 72
      },
      {
        "field": "_lastReportedStatus",
        "offset": 76
      }
    ]
  },

and

Conclusion

16B reduce per object.

Without PR

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
• I signed the CLA.
• I listed at least one issue that this PR fixes in the description above.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[flutter-dashboard]

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[jonahwilliams]

You'll need either a test or a test exception.

[fzyzcjy]

@jonahwilliams Thanks, sent https://discord.com/channels/608014603317936148/608018585025118217/1034242780857376799

[Hixie]

test-exemption: optimization

[jonahwilliams]

LGTM

[auto-submit]

auto label is removed for flutter/flutter, pr: 113927, due to - Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

[auto-submit]

auto label is removed for flutter/flutter, pr: 113927, due to Validations Fail.

[Hixie]