flutter test regression caused by copying field guard information to the Slot #47722
Comments
aam
added
the
area-vm
…guard information that is kept on a slot." This reverts commit 67baa58 as it breaks internal test b/205961373. Addresses #47722 TEST=ci Change-Id: Idc4a08a0a8cca2faf287a42eab8ff6af43f4e6c6 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/220484 Reviewed-by: Slava Egorov <vegorov@google.com> Commit-Queue: Slava Egorov <vegorov@google.com>
|
I presume the cherry pick request for this #47710 needs to be withdrawn as the change has been reverted. |
|
I think one issue with original change was that it relied on the fact that field's guard info can be captured on the Slot and used throughout compilation pipeline consistently. Here is a flow graph for the scenario in question(with Perhaps use of Slot's FieldGuardInfo need to be limited to unopt compilation pipeline. cc @mkustermann |
|
@aam If the compiler executes If the compiler now doesn't rely on information from the clone but exclusively on information from the => If we do this, the effect would be that after running the compiler, which disables field unboxing, the code installation check would discard the compilation due to relying on outdated information. It does seem a little safer than having some compiler passes see the |
Code I pointed out above(https://github.com/dart-lang/sdk/blob/main/runtime/vm/compiler/jit/jit_call_specializer.cc#L209) updates both clone and original fields. That allows current optimizing compiler to install new optimized code it produces. |
That is correct, it would drop the optimized compilation. About the suggestion on either using /cc @mraleph @alexmarkov opinions? |
|
Dropping the optimized compilation when the heuristic for boxing fields triggers might hurt performance much more than just not having this heuristic. To summarize the current state:
We have a trade-off between having this heuristic and better (simpler) compiler architecture. I think we should investigate if this heuristic for boxing fields bears its weight. Does it provide performance on golem benchmarks including dart2js and analyzer? Without this heuristic the overall architecture would be simpler (FieldGuardState will be immutable). If we want to keep this heuristic but make field guard state immutable throughout compilation, then we can also consider moving the heuristic out of compilation somewhere (check it before compilation / before Slot is created for the first time / some place in runtime outside of compiler / etc). |
|
I really don't like this heuristic - it is only there to improve performance on the code which mostly reads from double or simd fields and does not write to them as much - and does reading from unoptimized code. Unboxing such fields leads to excessive allocation. I highly doubt it improves performance of anything in our corpus but a bunch of benchmarks. Though there might be users out there which would see regressed performance if we disable this without implementing interprocedural unboxing in JIT mode. I think the way to fix this is the following: we should move this heuristic to the point where the first canonical slot is created for the given field. We make decision - disable unboxing on the original field if necessary then create a slot out of it. This way slot itself is immutable and heuristic still exists. For future safety we should also add a consistency check at the end of compilation (after making decision to commit) which would trigger if slots got out of sync with fields - they should not. |
|
With heuristic turned off(so that in jit unboxed fields never go boxed) I don't see significant regression on perf benchmarks. Only thing that stands out is NBodySIMD improvement of over 30% |
|
It can be that benchmarks that benefited from the heuristic were not ported from Dart 1 to Dart 2. (I am thinking it might have been Box2D). |
If I understand correctly Misc/Box2DOctane is what used to be Box2D and it doesn't show any change on removal of this heuristic on ia32 and x64, small regression on armv7(-0.1) and armv8(-0.3). |
|
I'd say then maybe we should just rip this heuristic out. I can artificially construct the example where it causes a regression (574ms -> 699ms), but it is super contrived. Noticed that @pragma('vm:never-inline')
void foo(double x) {}
class X {
double f;
X(this.f);
}
@pragma('vm:never-inline')
void bar(X x) {
X(1.9);
foo(x.f);
}
void main() {
for (var i = 0; i < 10; i++) {
final sw = Stopwatch()..start();
final x = X(0.1);
for (int i = 0; i < 100000000; i++) bar(x);
print(sw.elapsedMilliseconds);
}
} |
|
…k to boxed. The heuristic was introduced in b870daf, but gets in the way of maintaining Slot(extended with field guard information) immutable during compilation. Existing set of benchmarks doesn't seem to show any reasonable regression with this heuristic removed. Bug: #47722 Change-Id: Ie94050d96852b7ffef2dc81248cda23e84d41f4b TEST=ci Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/221544 Reviewed-by: Slava Egorov <vegorov@google.com> Reviewed-by: Ryan Macnak <rmacnak@google.com> Commit-Queue: Alexander Aprelev <aam@google.com>
Change that put field guard information on a slot(https://dart-review.googlesource.com/c/sdk/+/218987) resulted in flutter test failures in g3 b/205961373.
The text was updated successfully, but these errors were encountered: