Closed Bug 714841 Opened 8 years ago Closed 8 months ago

With 30 lightweight themes installed, Add-ons Manager appearance pane is slow to load

Categories

(Toolkit :: Add-ons Manager, defect, minor)

10 Branch
defect
Not set
minor

Tracking

()

RESOLVED WONTFIX
mozilla13

People

(Reporter: mkaply, Assigned: Unfocused)

References

Details

(Keywords: perf)

Attachments

(1 file)

Steps to reproduce:

Open Add-ons manager and go to appearance tab. Notice how fast it displays.

Install 30 personas.

Open add-ons manager. It takes 3-4 seconds.

I understand it would take longer, but this seems like a great deal of time.
Severity: normal → minor
Keywords: perf
Do you get the same issue with 30 extensions installed, and scrolling through them?
It's a lot harder to get 30 extensions installed :)

The issue wasn't scrolling, though, it was initially opening it.

I'll try to get 30 extensions installed and see.

I think it is much more likely a user would have 30 personas than 30 themes.
Sorry, I misread. Still, it would have helped narrow down the bottleneck. But I think I've found the bottleneck anyway.
Assignee: nobody → bmcbride
Status: NEW → ASSIGNED
Summary: With 30 personas installed, add-ons manager appearance is slow → With 30 lightweight themes installed, Add-ons Manager appearance pane is slow to load
Attached patch Patch v1Splinter Review
This makes a noticeable difference... 3sec before, now on par with listing extensions.
Attachment #596948 - Flags: review?(dtownsend+bugmail)
So was it parsing the JSON with each of the themes?
Attachment #596948 - Flags: review?(dtownsend+bugmail) → review+
It seems like the really important part of this fix is this:

-    aCallback([new AddonWrapper(a) for each (a in this.usedThemes)]);
+    let themes = this.usedThemes;
+    aCallback([new AddonWrapper(a) for each (a in themes)]);

Does the rest of the patch affect performance that much? Or is it really just about this part?
(In reply to Michael Kaply (mkaply) from comment #6)
> It seems like the really important part of this fix is this:
> 
> -    aCallback([new AddonWrapper(a) for each (a in this.usedThemes)]);
> +    let themes = this.usedThemes;
> +    aCallback([new AddonWrapper(a) for each (a in themes)]);
> 
> Does the rest of the patch affect performance that much? Or is it really
> just about this part?

Actually I glanced over this, I wouldn't expect that hunk to have any effect with the rest of the patch in place
> Actually I glanced over this, I wouldn't expect that hunk to have any effect with the rest of the patch in place

I was thinking the opposite. With just that patch in place, would the rest be needed?

The problem appears to be that when creating the addonwrappers it was calling usedThemes every time.

The fix for that would be exactly what was done, to put usedThemes in a local variable and use that to avoid hitting usedThemes every time.

The other fix, caching usedThemes seems like a second fix to the same problem.

So my question would be do either one of the fixes solve the problem alone, or are they really both needed?
(In reply to Michael Kaply (mkaply) from comment #8)
> So my question would be do either one of the fixes solve the problem alone,
> or are they really both needed?

Yeah I think only one is necessary. Caching usedThemes is going to have a larger impact than just solving the add-ons manager UI though, it potentially speeds up XUL window opening and anywhere else that we have to access usedThemes, so that seems like the better fix.
(In reply to Michael Kaply (mkaply) from comment #8)
> > Actually I glanced over this, I wouldn't expect that hunk to have any effect with the rest of the patch in place
> 
> I was thinking the opposite. With just that patch in place, would the rest
> be needed?
> 
> The problem appears to be that when creating the addonwrappers it was
> calling usedThemes every time.

No, looping over usedThemes to create AddonWrappers was only part of the problem. Fixing that alone didn't yield a dramatic speedup. Together with the other fix, it's merely a minor optimization.

In addition to usedThemes being accessed a lot directly, currentTheme is also accessed a lot (including when only using Add-on Manager APIs) - and that goes through the usedThemes property.
I didn't realize how many accesses to usedThemes there were and that the JSON gets parsed every time. That's crazy.

Caching is obviously the right thing to do here. Nice patch.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1e0a5cb0e79
Flags: in-testsuite-
Flags: in-litmus-
OS: Windows 7 → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/f1e0a5cb0e79
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Status: REOPENED → RESOLVED
Closed: 8 years agoLast year
Resolution: --- → INACTIVE
This is still an issue, though not as bad. If you install a lot of themes, when you click on the themes tab in about:addons, there's a noticeable delay until the themes appear.
Status: RESOLVED → REOPENED
Resolution: INACTIVE → ---

This appears to be specific to LWTs which are no longer supported.

Status: REOPENED → RESOLVED
Closed: Last year8 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.