This repository has been archived by the owner on Feb 20, 2023. It is now read-only.
Fact.toEvent is called many times & generally allocates 35 Pairs each time: power use issue #21294
Assignees
Labels
performance
Possible performance wins
Comments
This was referenced Sep 14, 2021
mcomella
changed the title
mcomella
changed the title
|
I ran the AS allocation tracker with the following step:
158 Pairs were allocated for a total of 2528 bytes (16 bytes per Pair – 8 byte / 64 bits per reference) on my Pixel 2. That doesn't seem too bad – there are much larger allocations going on. |
mcomella
added a commit
to mcomella/fenix
that referenced
this issue
Sep 18, 2021
…ode. When we refactor, this will help ensure we've done it correctly.
mcomella
added a commit
to mcomella/fenix
that referenced
this issue
Sep 18, 2021
mcomella
added a commit
to mcomella/fenix
that referenced
this issue
Sep 18, 2021
mcomella
added a commit
to mcomella/fenix
that referenced
this issue
Sep 18, 2021
mcomella
added a commit
to mcomella/fenix
that referenced
this issue
Sep 18, 2021
…RT_WEBEXTENSIONS.
mcomella
added a commit
to mcomella/fenix
that referenced
this issue
Sep 18, 2021
mcomella
added a commit
to mcomella/fenix
that referenced
this issue
Sep 18, 2021
mcomella
added a commit
to mcomella/fenix
that referenced
this issue
Sep 18, 2021
mcomella
added a commit
to mcomella/fenix
that referenced
this issue
Sep 18, 2021
mcomella
added a commit
to mcomella/fenix
that referenced
this issue
Sep 18, 2021
mcomella
added a commit
to mcomella/fenix
that referenced
this issue
Sep 18, 2021
mcomella
added a commit
to mcomella/fenix
that referenced
this issue
Sep 18, 2021
|
|
mcomella
added a commit
to mcomella/fenix
that referenced
this issue
Sep 22, 2021
…ode. When we refactor, this will help ensure we've done it correctly.
mcomella
added a commit
to mcomella/fenix
that referenced
this issue
Sep 22, 2021
…t.toEvent. This will not compile. However, it enables the subsequent PR to remove allocations from Fact.toEvent.
mcomella
added a commit
to mcomella/fenix
that referenced
this issue
Sep 22, 2021
This commit was generated primarily by a macro that: - appends `== component &&` - appends `== item` - (if applicable) Skips to the ending brace - Go down one line and move cursor to the front of the line to prep for repeat My only intervention was to skip extra lines to line it up to run again and specify how many times in a row it should run. --- The `to` in this code is an infix function that calls instantiates a Pair under the hood. Subjectively observed, when this method is called it generally hits the else case so 35 Pairs are instantiated each call - that's 560 bytes. This method is called frequently - for example, an estimated 4 times each time a letter is typed on the homescreen and a measured 116 times in a simple navigation (see the issue). The latter generates an estimated 63.4 KiB. It was straightforward to remove these allocations so that's what this change does. The primary risk from this change is that it's difficult to test each case to ensure it's working.
mcomella
added a commit
to mcomella/fenix
that referenced
this issue
Sep 22, 2021
These double comparisons are easier to read and see the pattern of on one line so I'd rather keep them on one line. Additionally, it's difficult to test each change individually so I'd rather not make additional changes. To do this, I suppressed the max line length warning.
mcomella
added a commit
to mcomella/fenix
that referenced
this issue
Sep 22, 2021
…ode. When we refactor, this will help ensure we've done it correctly.
mcomella
added a commit
to mcomella/fenix
that referenced
this issue
Sep 22, 2021
…t.toEvent. This will not compile. However, it enables the subsequent PR to remove allocations from Fact.toEvent.
mcomella
added a commit
to mcomella/fenix
that referenced
this issue
Sep 22, 2021
This commit was generated primarily by a macro that: - appends `== component &&` - appends `== item` - (if applicable) Skips to the ending brace - Go down one line and move cursor to the front of the line to prep for repeat My only intervention was to skip extra lines to line it up to run again and specify how many times in a row it should run. --- The `to` in this code is an infix function that calls instantiates a Pair under the hood. Subjectively observed, when this method is called it generally hits the else case so 35 Pairs are instantiated each call - that's 560 bytes. This method is called frequently - for example, an estimated 4 times each time a letter is typed on the homescreen and a measured 116 times in a simple navigation (see the issue). The latter generates an estimated 63.4 KiB. It was straightforward to remove these allocations so that's what this change does. The primary risk from this change is that it's difficult to test each case to ensure it's working.
mcomella
added a commit
to mcomella/fenix
that referenced
this issue
Sep 22, 2021
These double comparisons are easier to read and see the pattern of on one line so I'd rather keep them on one line. Additionally, it's difficult to test each change individually so I'd rather not make additional changes. To do this, I suppressed the max line length warning.
|
Simpler PR: #21446 |
mergify bot
pushed a commit
that referenced
this issue
Sep 30, 2021
When we refactor, this will help ensure we've done it correctly.
mergify bot
pushed a commit
that referenced
this issue
Sep 30, 2021
This will not compile. However, it enables the subsequent PR to remove allocations from Fact.toEvent.
mergify bot
pushed a commit
that referenced
this issue
Sep 30, 2021
This commit was generated primarily by a macro that: - appends `== component &&` - appends `== item` - (if applicable) Skips to the ending brace - Go down one line and move cursor to the front of the line to prep for repeat My only intervention was to skip extra lines to line it up to run again and specify how many times in a row it should run. --- The `to` in this code is an infix function that calls instantiates a Pair under the hood. Subjectively observed, when this method is called it generally hits the else case so 35 Pairs are instantiated each call - that's 560 bytes. This method is called frequently - for example, an estimated 4 times each time a letter is typed on the homescreen and a measured 116 times in a simple navigation (see the issue). The latter generates an estimated 63.4 KiB. It was straightforward to remove these allocations so that's what this change does. The primary risk from this change is that it's difficult to test each case to ensure it's working.
mergify bot
pushed a commit
that referenced
this issue
Sep 30, 2021
These double comparisons are easier to read and see the pattern of on one line so I'd rather keep them on one line. Additionally, it's difficult to test each change individually so I'd rather not make additional changes. To do this, I suppressed the max line length warning.
pkirakosyan
pushed a commit
to gexsi/user-agent-android
that referenced
this issue
Oct 1, 2021
…ode. When we refactor, this will help ensure we've done it correctly.
pkirakosyan
pushed a commit
to gexsi/user-agent-android
that referenced
this issue
Oct 1, 2021
…t.toEvent. This will not compile. However, it enables the subsequent PR to remove allocations from Fact.toEvent.
pkirakosyan
pushed a commit
to gexsi/user-agent-android
that referenced
this issue
Oct 1, 2021
This commit was generated primarily by a macro that: - appends `== component &&` - appends `== item` - (if applicable) Skips to the ending brace - Go down one line and move cursor to the front of the line to prep for repeat My only intervention was to skip extra lines to line it up to run again and specify how many times in a row it should run. --- The `to` in this code is an infix function that calls instantiates a Pair under the hood. Subjectively observed, when this method is called it generally hits the else case so 35 Pairs are instantiated each call - that's 560 bytes. This method is called frequently - for example, an estimated 4 times each time a letter is typed on the homescreen and a measured 116 times in a simple navigation (see the issue). The latter generates an estimated 63.4 KiB. It was straightforward to remove these allocations so that's what this change does. The primary risk from this change is that it's difficult to test each case to ensure it's working.
pkirakosyan
pushed a commit
to gexsi/user-agent-android
that referenced
this issue
Oct 1, 2021
These double comparisons are easier to read and see the pattern of on one line so I'd rather keep them on one line. Additionally, it's difficult to test each change individually so I'd rather not make additional changes. To do this, I suppressed the max line length warning.
|
Merged on Sep 30 in #21446 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Fact.toEventallocates a Pair for each comparison becausetois an infix function that creates a new Pair. The comparison tree is 35 items long and I subjectively noticed that most events that go through it will hit the worst case,else. To test the impact, I did a simple workflow:I added custom profiler markers and took a profile: https://share.firefox.dev/3k6vox3 Individually, these methods are fast enough that the user won't notice the impact. However, in aggregate, it's called 116 times in this simple workflow (estimated 116 * 35 = 4060 pairs * 16 = 64960 bytes) and takes 79.2ms (caveat: this duration may be inflated because these calls are fast and clock granularity might not be good enough). This is a non-trivial amount of work on the CPU that we suspect will impact power use (though we don't have the intuition to say exactly how much): we should optimize this method by reducing its runtime (e.g. removing the allocations), how often it's called, and/or removing the allocations to prevent later GC.
┆Issue is synchronized with this Jira Task
The text was updated successfully, but these errors were encountered: