Regression after removal of field lookup cache [265148324]

Fixed

Bug

Status Update

No update yet.

Description

ze...@google.com

created issue #1

Jan 11, 2023 03:59PM

The resolution of b/151804585 did not introduce a new field backing, thus lookups within a class are linear search in the field arrays. This has noticeable performance impact witnessed in b/264430795

Using the dump information provided in b/264430795, the initial regression came with CLs:

353672ms 78a3b2fe030b465c46c1b2f858cada93659a0e40 Minor cleanup prior to definitionFor(DexField) removal
335307ms 80ef5bef88b866f9dea82efc73b024ee751b42e2 Remove all uses of definitionFor(DexMember)
337779ms e7186a7da0d3edc84b389169614c60be4e208b14 Remove all uses of definitionFor(DexReference)
267364ms 225ddc66dc42ce0fdef363d17b61c88807b661ed Add CLI tests for various flag additions and fixes.

With the bottom CL being fairly equal in runtime to the original report at version 2.0.99.

Total regression is about ~1.5min on a previously 4.5min build (increase of about 33% in compiletime).

We should introduce a (potentially) hash-map based field collection backing mirroring the backing for methods.

Comments

ze...@google.com <ze...@google.com> #2Jan 11, 2023 04:20PM

A quick hack adding a field cache locally to each DexClass and lazily populating it on lookups confirms this regression. The hack is not correct as the cache is not consistent with changes to the fields so can't be used as a fix. It be close to the performance of an actual fix however.

On tip of tree the dump from b/264430795 has the times:

643616ms - Main @ 772d2f72524100b750eef6fc0d3c808efc2c3afd
435852ms - Field lookup cache

which is pretty close to the same relative regression as on the 2.1 branch.

(The time on main is still larger in total time than on 2.0.99 which may be other regressions or expected compile time cost from new optimizations.)

ap...@google.com <ap...@google.com> #3Jan 13, 2023 12:02PM

Project: r8
Branch: main

commit 821b111d86b14b78910ac8b8bceb7bfe2bc04508
Author: Ian Zerny <zerny@google.com>
Date: Fri Jan 13 13:00:14 2023

Introduce a field collection with array and map-based backings.

Bug:

b/265148324

Change-Id: I5155084088ce04e389073314d763b4a952610add

M src/main/java/com/android/tools/r8/graph/DexClass.java
M src/main/java/com/android/tools/r8/graph/DexClasspathClass.java
A src/main/java/com/android/tools/r8/graph/DexFieldSignature.java
M src/main/java/com/android/tools/r8/graph/DexProgramClass.java
A src/main/java/com/android/tools/r8/graph/FieldArrayBacking.java
A src/main/java/com/android/tools/r8/graph/FieldCollection.java
A src/main/java/com/android/tools/r8/graph/FieldCollectionBacking.java
A src/main/java/com/android/tools/r8/graph/FieldMapBacking.java
M src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxingTreeFixer.java

https://r8-review.googlesource.com/75149

ap...@google.com <ap...@google.com> #4Jan 16, 2023 11:14AM

Project: r8
Branch: 8.0

commit 6d14908a652431a6597c5462587568b6937ed9fe
Author: Ian Zerny <zerny@google.com>
Date: Mon Jan 16 10:43:25 2023

Version 8.0.26

Bug:

b/265148324

Change-Id: I18eec5e48e167f9e0b951660fe8934c37357f228

M src/main/java/com/android/tools/r8/Version.java

https://r8-review.googlesource.com/75155

ap...@google.com <ap...@google.com> #5Jan 16, 2023 11:14AM

Project: r8
Branch: 8.0

commit 9f5d05983b53811f73c2854e7bf8c928e14daf37
Author: Ian Zerny <zerny@google.com>
Date: Mon Jan 16 10:43:08 2023

Introduce a field collection with array and map-based backings.

Bug:

b/265148324

https://r8-review.googlesource.com/75154

ze...@google.com <ze...@google.com> Jan 18, 2023 08:37AM

Marked as fixed.

Issue 265148324

Description

Issue summary

Comments

ze...@google.com <ze...@google.com> #2Jan 11, 2023 04:20PM

ap...@google.com <ap...@google.com> #3Jan 13, 2023 12:02PM

ap...@google.com <ap...@google.com> #4Jan 16, 2023 11:14AM

ap...@google.com <ap...@google.com> #5Jan 16, 2023 11:14AM

ze...@google.com <ze...@google.com> Jan 18, 2023 08:37AM

Add comment

Issue metadata