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

As I understand it, we need to migrate namespace definitions both from AndroidManifest.xml and from implicit definitions from applicationId / testApplicationId? Is that right? So an 8.0 project would have a

namespace <n>
testNamespace <tn>

where <n> is taken from AndroidManifest.xml package definitions if present, and applicationId if not, and <tn> is testApplicationId if present and <n>.test if not?

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

Sorry for the confusion. AGP would change how it computes the namespace for the androidTest variant. All the Upgrade assistant would need to do is update the source code for the import of the R class.

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

OK! And, I think, this should happen at the same time as the namespace declaration is migrated from AndroidManifest.xml?

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

It's unrelated, but it is possible that we move 8.0 to also only declare the namespace into the DSL. I would make it a separate refactoring because they are not tied at all.

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

Marked as fixed.

Issue 265148324