improve dead/redundant store elimination

[mraleph]

At some point we were considering to allocate objects uninitialized - however this is not the case at the moment so any store of null into newly allocated object is in fact dead. This issues consists of two several items:

• Initializing stores of null can simply be dropped (e.g. in the canonicalization pass)
• LoadOptimizer can be taught to remove redundant stores (e.g. storing value into a field which already has this value is redundant). This would remove non-initializing stores of null into newly allocated objects.
• StoreOptimizer::CanEliminateStore should be revisited because it currently does not eliminate initializing stores - which seems wrong.
• Context objects can be allocated uninitialized as a performance optimization (all initializing stores are inlined into he caller, which allocates the context). Investigate if this can be changed to align with normal Dart objects for code size reasons. AOT does not lower context allocation - only JIT does.

/cc @mkustermann

[mraleph]

/cc @mkustermann

[mraleph]

Simple removal of redundant initialising stores have landed and has the following effect

Benchmark	Change	Metric	Target	Hardware	Arch
flutter_gallery_instructions_size	-1.05 %	CodeSize	flutter-profile	Moto G4	android-armv7
flutter_gallery_instructions_size	-1.29 %	CodeSize	flutter-release	Moto G4	android-armv7
flutter_gallery_instructions_size	-1.23 %	CodeSize	flutter-release	Pixel 2	android-armv8

[aartbik]

Given the guaranteed clearing semantics of allocation, I am looking into D/R(S/L)E improvements that will eliminate both a = null statements from code like this (currently we only remove one, solely based on kill sets). There may be some other low hanging fruits.

class Bar {
  Bar() { a = null; }
  Object a;
}

@pragma("vm:never-inline")
Bar foo() {
  Bar bar = new Bar();
  bar.a = null;
  return bar;
}

[mraleph]

I am looking into DSE improvements

I have been actually pondering on this topic a bit - which made me wonder if our DSE should be merged in the LoadOptimizer which can be made to compute all the necessary information that DSE needs. Additionally LoadOptimizer already computes information about the state of the object fields (for load forwarding) and this is exactly the information that you need to discover redundant stores (i.e. store that stores a value which is already in the field). So it seems natural that this should be handled in the same pass.

Given the guaranteed clearing semantics of allocation

I wonder if item nr 4 on the list (null-initialized context allocations - and potentially uninlining context cloning) might yield better size savings then more sophisticated DSE. (at least it looks easier to crunch some numbers on this).

[aartbik]

@mraleph I have implemented the redundant store removal in the LoadOptimizer, and agree that DSE is almost a special case of this optimization now. We can do that refactoring as a follow-up.

[aartbik]

Redundant store elimination yields very modest saving of 13K bytes.

+---------------------------+---------------------------+--------------+
| Library                   | Method                    | Diff (Bytes) |
+---------------------------+---------------------------+--------------+
| dart:collection           | MapMixin.entries          |          -64 |
| package:flutter/src/wid.. | _AnimatedPhysicalModelS.. |          -64 |
| package:flutter_gallery.. | FullScreenDialogDemoSta.. |          -64 |
| package:flutter_gallery.. | ContactsDemoState.build   |          -80 |
| package:flutter_gallery.. | _ChipDemoState.build.<a.. |          -80 |
| package:flutter_gallery.. | _DataTableDemoState.build |          -80 |
| package:flutter_gallery.. | _ExpansionPanelsDemoSta.. |          -96 |
| package:flutter/src/wid.. | GestureDetector.build     |         -112 |
| package:flutter/src/wid.. | _AnimatedContainerState.. |         -112 |
| package:flutter_gallery.. | _DataTableDemoState.bui.. |         -128 |
+---------------------------+---------------------------+--------------+
Comparing ./old.json (old) to ./sizes.json (new)
Old   : 6964496 bytes.
New   : 6950816 bytes.
Change: -13680 bytes.

Some more data.

When naively doing point (3) above (which actually seems required for backward flowing DSE since we want to keep the first; I did not need it for the forward flowing CSE), this only saves an additional 16 bytes compared to the optimized version above.

Removing the two bail-outs in load/store optimizer (which seemed to have been put in place to avoid OOM for some unspecified situations) has no impact, as these cases are not encountered for the gallery.

[aartbik]

All four points have been addressed.

[yaymalaga]

Should this issue be opened again due to c4a7f1c?

[mraleph]

We have relanded the reverted change in: https://dart-review.googlesource.com/c/sdk/+/121982

I forgot to connect it to this issue though.