Open Bug 1523028 Opened 6 years ago Updated 2 years ago

[meta] Performance of "about:config"

Categories

(Toolkit :: Preferences, defect, P3)

defect

Tracking

()

Performance Impact medium
Tracking Status
firefox-esr60 --- unaffected
firefox66 --- unaffected
firefox67 --- wontfix
firefox67.0.1 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 --- wontfix
firefox72 --- wontfix
firefox73 --- wontfix

People

(Reporter: smaug, Unassigned)

References

(Depends on 5 open bugs, Blocks 1 open bug)

Details

(4 keywords)

Attachments

(1 obsolete file)

aboutconfig.html checkerboards quite badly when scrolling on this
linux laptop. i7-4750HQ CPU, using Intel GPU, 3840x2160 display.

Scrolling the old about:config is smooth (not surprising, since it is using rather well optimized widget for showing the large set of data).

Thanks for reporting! Worth turning this into a Graphics bug, which will be useful to fix unless this is specific to pages being loaded in the parent process.

It's great to see how we can use our internal pages to dogfood our platform code using real patterns that are much more common on the real web, like tables of about 3,000 rows, instead of living in a silo of heavily optimized widgets.

Also needinfo Mike to take a first look at the profile.

Flags: needinfo?(mconley)

Well, hopefully we can also change aboutconfig.html to be faster to render. It is not that that kind of page is common in the web.

~120ms per rasterize is pretty horrifying. smaug, if you enable page flashing, it's not trying to paint the entire content area each time, is it?

Component: Preferences → Graphics
Flags: needinfo?(mconley) → needinfo?(bugs)
Product: Toolkit → Core
Whiteboard: [qf]

The issue happens when scrolling fast using scrollbar, so it of course is painting quite a lot.

Flags: needinfo?(bugs)

I'm seeing this as well (over-100ms rasterize operations), on my ThinkPad P50 with a maximized Firefox Nightly window, scrolling large distances by dragging the scrollbar.

My resolution is 3840x2160 (and I've got 200% HiDPI scaling).
My profile: http://bit.ly/2Rmu6fL

"Home/End" keypresses are another easy way to trigger this.

WebRender doesn't seem to help, either -- I still see flashes of blank content when I jump up & down. Profile with that: http://bit.ly/2RmvWNH

Here's another profile with webrender enabled, zoomed to a slow "Press 'End' [wait for paint] Press 'Home' [wait for paint]" keypress scenario:
http://bit.ly/2Rrj7Se

Given that this is a giant <table> & the profiles show display list generation during the slow parts, this seems possibly related to bug 1456638 ("Checkerboarding while scrollbar-dragging due to slow display list build on specific page with table layout")

Summary: Scrolling aboutconfig.html checkerboards badly → Scrolling aboutconfig.html checkerboards badly (janking when rasterizing table
Summary: Scrolling aboutconfig.html checkerboards badly (janking when rasterizing table → Scrolling aboutconfig.html checkerboards badly (janking when rasterizing table)
Whiteboard: [qf] → [qf:p2:responsiveness]

http://bit.ly/2CQoDZs is a profile of opening the new about:config and typing '.' in the filter box, on a fast i9 macbook pro. Is the 340MB increase in memory usage also a concern here?

Definitely it should be.

(xul:tree really is optimized for performance and memory usage, so I do expect that we'll need something similar for aboutconfig.html. Gecko profiler has rather fast html tree widget, and doesn't devtools too)

It's really rather unfortunate that our engine can't efficiently handle an HTML table this large. :/

The problem is made worse since all of this runs in the parent process for now.

Paolo - while I'm aware of the accessibility advantages of using a standard table here, I'm rather worried about the end-user experience. about:config is an internal page that we have total control over, and failing to render it smoothly in some cases isn't a great look.

You wrote in your email:

We've explicitly chosen to avoid virtualizing the list, that is only
rendering visible DOM nodes, because this would add complexity that is
not needed for an internal page. It would also nullify most of the
advantages in accessibility and usability that we gained at a low cost
just because we're using a simple HTML table. Effort would be better
spent on optimizing the web for the layout of tables of about 3,000
rows, which would benefit every web site instead of Firefox only.

I agree with you wholeheartedly in principal - we should definitely aim to improve table rendering for the web at large (bug 1456638 is likely to help here) - however, I suspect that bug is unlikely to be prioritized since I believe most of the relevant folks are heads down on either WebRender or page load performance.

Do you have a sense of how much effort it would be to virtualize the list? Are the tree widgets from the Gecko Profiler or DevTools re-usable here?

Flags: needinfo?(paolo.mozmail)

(In reply to Mike Conley (:mconley) (:⚙️) from comment #12)

The problem is made worse since all of this runs in the parent process for now.

AIUI moving this into the content process was a non-goal, since we didn't want to add a way to read and write arbitrary prefs from the content process. Is that something that the upcoming chrome-priv content process would help with?

(In reply to Mike Conley (:mconley) (:⚙️) from comment #12)

Are the tree widgets from the Gecko Profiler or DevTools re-usable here?

I asked in devtools #perf channel:

do you know if it's something that could be vendored out and reused or is it pretty specific to the perf-html UI?

And heard back:

have you looked at react-virtualized-list?
ours is completely custom, but is maybe too complex for the needs of about:config
https://github.com/devtools-html/perf.html/blob/master/src/components/shared/VirtualList.js <= this is our virtual list implementation

(In reply to Brian Grinstead [:bgrins] from comment #13)

(In reply to Mike Conley (:mconley) (:⚙️) from comment #12)

The problem is made worse since all of this runs in the parent process for now.

AIUI moving this into the content process was a non-goal, since we didn't want to add a way to read and write arbitrary prefs from the content process. Is that something that the upcoming chrome-priv content process would help with?

You're right that we don't want normal web content processes being able to set arbitrary prefs (or send messages to set arbitrary prefs). I do think that something like the privileged content process is the right place to host in-content about:config down the line. (Same with about:preferences).

(In reply to Brian Grinstead [:bgrins] from comment #14)

have you looked at react-virtualized-list?

This is the widget they were referencing: https://bvaughn.github.io/react-virtualized/#/components/Table

(In reply to Mike Conley (:mconley) (:⚙️) from comment #12)

Do you have a sense of how much effort it would be to virtualize the list?

Very high in comparison to the size of this code, as in we'll probably have to rewrite the entire page and all the regression tests.

Are the tree widgets from the Gecko Profiler or DevTools re-usable here?

Victor and Brian have more context on this, but from a brief conversation with them it seems that at least the DevTools widgets are more specialized than they might seem on the surface. As far as I can tell, if you're willing to accept that you have to rewrite "about:config" almost entirely to use them, then you have to figure out things like how to share styles, knowing that DevTools has special theming support, whether there is a dependency on React, and whether you have to change where the code is located and possibly its ownership for future maintenance.

I don't know how those widgets handle accessibility at present, and they might have special code for it, but features like find in page or displaying the full values will be impacted, and alternate ways of accessing the same features might have to be coded.

While tackling all these questions just for "about:config" seems too much effort, your questions touch on the general matter of what we'd like to do with tree removals going forward, so there might be some convergence of intent that might benefit this page in the future. Brian and Victor might be able to elaborate on this.

It's also worth noting that this was page initially written using ContentTask for tests, with the idea of moving it to a dedicated content process with special API access. This was then ruled out because support for this was far in the future of the Fission roadmap, but it's another thing that might improve performance. If a high-effort rewrite of this page ever happens, this is something that should be kept into account, to avoid the need of a third rewrite.

Finally, there's a lot of attention on this page specifically at the moment because it is new, but in everyday use the impact of checkerboarding might be lower than "painted" on this bug ;-)

Ah, I see that part of my reply was already covered :-)

Flags: needinfo?(paolo.mozmail)

Setting needinfo in case Brian or Victor have anything to add in relation to comment 17.

Flags: needinfo?(vporof)
Flags: needinfo?(bgrinstead)

Some previous discussions on the accessibility of the VirtualizedTree component:

https://bugzilla.mozilla.org/show_bug.cgi?id=1335055

A couple of tricky parts with the virtualized table widgets that I've looked at in the past are:

  1. 'find in page' doesn't work
  2. screen reader support is poor

For (1): that would be the status quo with the old about:config UI.

For (2): Marco, have you analyzed any of these widgets before? Or if not, could you please provide feedback on the accessibility support for a widget like https://bvaughn.github.io/react-virtualized/#/components/Table? I do see a number of issues flagged when searching https://www.google.com/search?q=react-virtualized+screen+reader.

Flags: needinfo?(bgrinstead) → needinfo?(mzehe)

Well, the table itself looks semantically correct with a screen reader, except the table headers, which are text inputs, for some reason. The actual data rows are text or such, at least in the example you linked to above.

However, keyboard navigation either does not work, or if it does, it does not emit any accessible focus events. I can tab to some of the table headers, and a grouping where the data is presumably presented, but then, arrow keys don't echo anything.

This widget appears to not be what we want.

Flags: needinfo?(mzehe)

Compare Fastlist

Before any attempt is made to replace uses of XUL <tree> with HTML, we first need a proper replacement HTML component that can do the same thing as XUL <tree>, and just as fast and CPU- and memory-efficient. Only after we have such a component can we use HTML to replace XUL <tree>s. The component can be written in HTML and JS, but it is far from trivial.

I was asking around in #accessibility, and it turns out :yzen is working on a a11y-friendly list component as part of Bug 1518487. It's not virtualized (due to wanting variable height rows), but nothing would stop it from becoming virtualized or porting the a11y from it.

(In reply to Mike Conley (:mconley) (:⚙️) from comment #12)

Paolo - while I'm aware of the accessibility advantages of using a standard table here, I'm rather worried about the end-user experience. about:config is an internal page that we have total control over, and failing to render it smoothly in some cases isn't a great look.

FWIW, it's pretty horrible for a11y too at present. Pressing escape to show the entire table causes a freeze of about 15 seconds with Firefox + NVDA with a Thinkpad P50 (quad core Xeon). I originally thought this was due to our hideous a11y e10s perf issues, but now that I realise this is in the parent process, I'm much more worried. In short, showing the entire table is unacceptable from a perf perspective with screen readers.

Is it possible to maybe introduce paging (for both search results and viewing all prefs) using the first letter of the pref ?

Paolo original's email states that the original UX was the easiest one to implement that meets all the requirements. So can't this be done here too ?

Paging should be ok in terms of performance/memory/a11y, and doesn't sound as challenging as implementing a virtualized view or integrating react into the UI.

(In reply to Tim Nguyen :ntim from comment #27)

Is it possible to maybe introduce paging (for both search results and viewing all prefs) using the first letter of the pref ?

This is one of the options that removes the ability to display all the preferences at once, which involves a number of other trade-offs for user experience. An easier implementation could require three characters minimum for search and add, and based on my tests it would achieve a similar effect by showing 892 preferences in the worst case ("ser"), compared to 650 in the worst case with paging.

If I understand correctly, Mike's concern in comment 12, shared by others on this thread, is that this page could be considered a "visit card" for Firefox: "about:config is an internal page that we have total control over, and failing to render it smoothly in some cases isn't a great look."

My take on the above is that showing all preferences at once is really an insider use case, which can indeed be useful, but different from the task that most end users will perform here. I also expect the time spent on this page by 99.5% of users to be a few minutes every year - and I don't think we need telemetry data to agree on that.

The remaining 0.5% is us, and we use this page a lot. Knowing us, we would like to spend as much time as we can to live in a world where we can see the best functionality, performance, accessibility, user experience design, and code quality in everything we make, because we pride ourselves in being the best at delivering those experiences. We know very well that managing effort and having higher development speed are important, but they're more transient qualities that don't stay in the final product, so it's more difficult to accept that they're part of the trade-offs. This is not unique to this feature, it is the case with every feature we develop, but in my experience it's more difficult to take an objective view when we have a pre-existing idea of what the trade-offs should look like.

The old "about:config" page suffered with maintainability (including the inability to make changes as we were inextricably linked to what the "tree" widget offered us), with user experience design, and with code quality (including lack of regression tests). The functionality was also severely limited - and I'm not speaking of the count of possible interactions, but how useful they end up being in practice. It had good performance at the expense of other aspects.

Now we have a new experience that offers new trade-offs, including future expansion that would've been quite expensive before. We should be deliberate in accepting these trade-offs, that's why I definitely welcome all the feedback here, especially Mike's since front-end performance is his main focus. I feel confident that the new trade-offs are better, but if Mike feels this needs more work, we can put more effort in ensuring it meets expectations. If this requires more engineering effort, we should discuss with engineering owners and managers, and if this requires removing functionality, we should discuss with the product owners.

Two solutions that come to mind here for performance might be to continue supporting the ability to show all preferences, but make it less discoverable, or clearly separate it with a smaller "Show All" link at the bottom of the empty page, with possibly a "confirm" dialog that makes it clear upfront that it may be slow.

I don't particularly like any of these solutions, including pagination, because it seems to me that they solve some ephemeral needs of our small group of early testers, instead of the main task of 99.5% of final users or routine tasks of Nightly users. We're discussing the 1% of cases where we have a use for displaying all preferences at once. Interestingly, that 1% of 0.5% is also one of the first things early testers would do to explore the page.

I do think, however, that acknowledging the performance and resource limitations we encountered here is useful forward thinking. At the moment, we should definitely continue to gather input. As far as "about:config" goes, we can discuss and then review in one week or so to see what the next steps could be.

Given how much effort is ongoing related to improving performance in Firefox and ensuring Firefox is perceived as fast, I don't think regressing performance so significantly can be called a "trade-off".

You seem to consider the case of displaying all preferences at once an edge case that's not worth focusing on, so let's look at the performance of typing in the searchbox to look for a specific set of preferences: http://bit.ly/2CUvqRY

In this profile, I typed "browser" slowly. As a user, I expect this to behave like the awesomebar that filters results, and where I need to type one more letter to filter further.

A few observations from looking at the profile and a little bit at the code:

  • the updates seem to be triggered by idle callbacks with a 500ms timeout. Idle callbacks are not a good fit for things triggered by user actions. I think what you want to do here is ensure that if the user is typing quickly, we don't refresh the UI once per key event. You could achieve this with a requestAnimationFrame callback or by using Services.tm.dispatchToMainThread; without imposing a long delay.
  • There's A LOT of time spent removing DOM nodes when setting textContent to an empty string to clear the view. There must be a more efficient way to do this, and if there isn't we should talk to DOM people to see if they can help.
  • Layout takes a long time too. I haven't verified, but I suspect this is due to having rows of variable height depending on their content. Seeing the long values in the xul about:config was difficult; it's good to make this easier, but needing to see the whole value is an edge case; I don't think we need to optimize for it. Could we consider showing the pref value on a single line with an ellipsis, and use a double click to show the whole value?
  • I don't think we need to have the whole content in the DOM to let users search. I think you wanted to do this so that the normal findbar would be usable. IIRC the xul about:config filter box was filtering on both pref names and pref values, so that replaced the findbar well. Can we do the same here?

Finally, performance is not just a matter of raw numbers, but is also about how long it takes the user to complete his task. For example, bug 1524080 wastes me as much time as all the other performance issues I mentioned before, because right after typing 'about:config<enter>', I'm ready to use the keyboard more to press that button, and then type into the searchbox. Needing to switch to a mouse interaction to click the unfocused button and then back to the keyboard to search wastes me about 2s each time.

The common reason for me to look at the whole list of preferences is to look for preferences that might have been affected by malware. I think this is a common case for people using about:config to troubleshoot misbehaving instances of Firefox on other people's computers. We could significantly improve the performance for this use case by showing all the modified preferences by default. That would be a short enough list to have it displayed quickly even without fixing the other performance issues. I'm not saying we should do this by the way, just giving an example of how changing the default behavior could affect the perception of performance without fixing the actual technical issues.

As a summary, I think the current HTML version of about:config is not ready to ship, but most of the problems seem actionable. Even for nightly, it seems a bit rough, so I would suggest disabling it for now (maybe in a couple days if you think we'll gather feedback that we haven't received yet) to give time to address the feedback without pressure.

I don't think regressing performance so significantly can be called a "trade-off".

Agreed. Also, developer productivity is important, because slow developers mean less product features.

More importantly, I see that as a highly useful product feature that's being wilfully destroyed. This is a regression. We do not accept regressions. And users hate regressions. It's a mistake to write off power users as a minority. It's that very minority of power users that made Firefox grow and be in the place where it is. If you destroy their favorite features and make the feature a laughing stock, then this will affect the market share of Firefox.

There is a bigger underlying problem here: This is an attempt to replace XUL <tree>, before creating a suitable replacement. This is a mistake. First, the widget set needs to be created that supports features like this, and only then can you create the application-level features that use it.

Creating a fast list or tree that can show thousands of entries quickly, and update the view quickly, is a major engineering task. In Gecko, it was the core developers that implemented XUL <tree>. It is our most complex and most advanced widget. All this complexity is being ignored and washed away as "we don't need all that". This is worry-some, because there will be many more uses of XUL <tree> that are much more prominent, and they will suffer in the same way, unless we first implement a proper base widget that can fulfill the requirements. This is not a task for an undergraduate.

[Tracking Requested - why for this release]:
Performance issue

(In reply to Ben Bucksch (:BenB) from comment #31)

More importantly, I see that as a highly useful product feature that's being wilfully destroyed. This is a regression. We do not accept regressions. And users hate regressions. It's a mistake to write off power users as a minority. It's that very minority of power users that made Firefox grow and be in the place where it is. If you destroy their favorite features and make the feature a laughing stock, then this will affect the market share of Firefox.

Talking about this as being willfully destroyed and a "laughing stock" isn't productive or fair to anyone who worked on this. It's also overstating the problem and ignoring the fact that we are actively discussing how to solve it right here, in this bug.

There is a bigger underlying problem here: This is an attempt to replace XUL <tree>, before creating a suitable replacement. This is a mistake. First, the widget set needs to be created that supports features like this, and only then can you create the application-level features that use it.

To be clear, we aren't currently removing the XUL tree widget itself from m-c. We're in the process of replacing the XBL bits of tree with Custom Elements (see Bug 1523957 and blockers), so it'll continue to work post XBL and until we don't need it anymore.

What we have been doing is removing individual XUL tree instances that don't require the featureset they provide (see Bug 1446335 and the landed blocking bugs). Usually these are lists with no nesting and a bounded number of elements, where we can use a simpler widget / element (<xul:richlistbox> or <html:table>). As discussed here, it looks like ~3K rows was above the upper bound of what that approach can handle given the platform constraints, which is why we are talking about alternatives either on the widget side or on the about:config UI side.

willfully destroyed and a "laughing stock" ... fair to anyone who worked on this.

You're right. That wasn't fair, and I apologize.

As discussed here, it looks like ~3K rows was above the upper bound

For HTML tables, I'd use 100 as upper bound.

What we have been doing is removing individual XUL tree instances that don't require the featureset they provide

I understand that. What I'm saying is that before you attempt to replace XUL <tree>, you first need to make a suitable replacement. It's fair to first build a widget that e.g. can only do lists and not hierarchical trees. But the widget needs to be there first, and needs to be designed from the get-go with the features that it needs to eventually support, otherwise we end up with chaotic APIs and widgets that cannot support important use cases does to an unfortunate API. In this case, we don't need to start from scratch, we know what's needed, so we can design the API from properly, and build the features as needed.

What I think is wrong is to start replacing XUL trees in an ad-hoc fashion without a replacement widget and with pure HTML. That will fail. And it does fail here.

This implementation is missing (you can see that right from the start in the first minute):

  • Lines are too high, wasting screen real estate (easy to fix even in HTML)
  • Double-click to toggle/edit does not work (easy to fix even in HTML)
  • Search bar misses clear button (easy to fix even in HTML)
  • Not enough space for values (easy to fix even in HTML)
  • Values lines wrap (means variable line length, which kills performance)
  • Context-menu missing (harder)
  • Column headers missing. Sort impossible. (hard)

Most of these things are trivial to fix. The column headers are not. They require a new data-driven widget. That's why I said that this needs to come first.

You first need the platform and widget set before you can implement application features. The thing that bothers me is that this starts the wrong way around.

(In reply to Ben Bucksch (:BenB) from comment #34)

As discussed here, it looks like ~3K rows was above the upper bound

For HTML tables, I'd use 100 as upper bound.

What we have been doing is removing individual XUL tree instances that don't require the featureset they provide

I understand that. What I'm saying is that before you attempt to replace XUL <tree>, you first need to make a suitable replacement. It's fair to first build a widget that e.g. can only do lists and not hierarchical trees. But the widget needs to be there first, and needs to be designed from the get-go with the features that it needs to eventually support, otherwise we end up with chaotic APIs and widgets that cannot support important use cases does to an unfortunate API. In this case, we don't need to start from scratch, we know what's needed, so we can design the API from properly, and build the features as needed.

What I think is wrong is to start replacing XUL trees in an ad-hoc fashion without a replacement widget and with pure HTML. That will fail. And it does fail here.

I believe all the other ones that we've replaced in this manner have been quite smaller and we haven't seen any problems with the approach in general until here.

This implementation is missing (you can see that right from the start in the first minute):

  • Lines are too high, wasting screen real estate (easy to fix even in HTML)
  • Double-click to toggle/edit does not work (easy to fix even in HTML)
  • Search bar misses clear button (easy to fix even in HTML)
  • Not enough space for values (easy to fix even in HTML)

Sure, without going into each one those sound like reasonable changes that could be broken down into individual bugs.

  • Values lines wrap (means variable line length, which kills performance)

I see the variable line length as a nice feature (as mentioned in Comment 30), although it well may be something we have to drop for perf.

  • Context-menu missing (harder)

I originally thought the same thing about the missing context menu, but after using it for a bit I got used to not having it. Other in-content pages (i.e. about:preferences) don't even have a context menu at all (while this page has the normal web context menu that lets you copy selection, search, etc).

  • Column headers missing. Sort impossible. (hard)

Most of these things are trivial to fix. The column headers are not. They require a new data-driven widget. That's why I said that this needs to come first.

FWIW: Column headers and sorting can be done with <richlistbox> (although not with a plain HTML table).

I copy/pasted the markup for the prefs table from the UI but with no JS / CSS into https://satin-dime.glitch.me/. Jamie, I'm curious if you are seeing the 15 second freeze with that page as well? Just trying to narrow down if it's related to the amount of content in the table, or if it's something particular with the JS / CSS in the new about:config page.

Flags: needinfo?(jteh)

FWIW: Column headers and sorting can be done with <richlistbox> (although not with a plain HTML table).

Yeah, but <richlistbox> is a) also XUL and b) is not suited for large tables and suffers from the same performance problem, for the same reason: Variable line height.

To get decent performance, you need to virtualize the rows. And to do that, you need a completely different widget API and programming approach. That's why I said that needs to be done first.

Once you have the rows as pure data, then sorting becomes easy, too.

Sorting is important. For example, I often sort by "Status" to see only the prefs that are modified. This is an essential feature of about:config.

I'd also like to get some measurements on this page when it's running in the chrome-priv content process instead of the parent process. Mike, is that something that could already be wired up locally, or will we have to wait on some more blockers on Bug 1513045 to be resolved first?

Flags: needinfo?(mconley)

(In reply to Ben Bucksch (:BenB) from comment #37)

FWIW: Column headers and sorting can be done with <richlistbox> (although not with a plain HTML table).

Yeah, but <richlistbox> is a) also XUL

It's tangential to the rest of the discussion here, but richlistbox is a lot closer to being able to be converted into an HTML element than tree is. There's no custom XUL element subclass required for richlistbox to work, so we could ultimately have it extend HTMLElement in frontend code.

(In reply to Ben Bucksch (:BenB) from comment #24)

Compare Fastlist

Before any attempt is made to replace uses of XUL <tree> with HTML, we first need a proper replacement HTML component that can do the same thing as XUL <tree>, and just as fast and CPU- and memory-efficient. Only after we have such a component can we use HTML to replace XUL <tree>s. The component can be written in HTML and JS, but it is far from trivial.

There is a bigger underlying problem here: This is an attempt to replace XUL <tree>, before creating a suitable replacement. This is a mistake. First, the widget set needs to be created that supports features like this, and only then can you create the application-level features that use it.

I'm one of the folks outlining and working on the XUL tree removal plan, orthogonal to what's happening here with about:config. There's a few points I'd like to bring here with regards to what's happening.

I agree that replicating a XUL tree using web technologies is far from trivial, especially when it comes to performance and accessibility.

Your comment about first needing a proper replacement that can do the same things as XUL trees is interesting. Let's consider whether or not that's true. I wrote a document[0] outlining all usage of these trees in the Firefox product, breaking down three kinds of usage scenarios for the consumers which we plan to support. Here's the most important aspects that we've learned:

  • The XUL tree component is powerful to a degree that is not required in almost all of Firefox nowadays. There's only a need for either simple trees, or simple tables with resizable columns, or just <richlistbox> elements, with no overlap for consumers: it’s not actually necessary to have complex tree-tables which are suitable for all possible use-cases.

  • When it comes to the necessity of virtualised lists (or some other optimisation strategies to account for vast amounts of presented data), there's only one kind of consumer in Firefox that requires this (apart from about:config), and that is Places. Every other XUL tree instance can switch over to utilise a rewritten simple tree, or transition over to a XUL <richlistbox>, because data sets are either small or deterministically very limited.

Given these present requirements in Firefox, the need to fully replicate existing XUL tree behaviour in a single widget is not vital. Multiple widgets, smaller in scope and specialised for the specific data they wish to present, are adequate. Long term, maintaining a handful of small+specialised local widgets instead of the single complex+generalised XUL tree widget is a great trade-off.

Tangential to all of this, Thunderbird does indeed utilise XUL tree capabilities to a vast extent. The decision to only consider Firefox use-cases moving forward when supporting internal widgets (and this includes XUL trees) has already been made. Removing XUL trees bindings and features (and converting them to Custom Elements) has been happening with proper notice, and we don't have short term plans for removing the platform XUL tree code.

To continue this meta-conversation, I suggest discussing the general aspects of XUL tree removal and planning in an appropriate channel, to keep this bug on-topic with regards to the new about:config page's performance or need of UX polish. We've already outlined the medium-term plan for removing XUL trees in Firefox in the document I mentioned above[0].

This implementation is missing (you can see that right from the start in the first minute):

  • Lines are too high, wasting screen real estate (easy to fix even in HTML)
  • Double-click to toggle/edit does not work (easy to fix even in HTML)
  • Search bar misses clear button (easy to fix even in HTML)
    ...

We should consider whether or not the neutral approach in this scenario is to consult with the appropriate UX and product people about the actual feature-set and design needed for about:config. It's likely that everybody has strong opinions (weakly or strongly held) about what power users want, and UX/product will make the best decisions here.

[0] https://docs.google.com/document/d/1lrWIAKTGfyO8_FaAScUc9V57jyZdM-odig00MmFh9HA

Flags: needinfo?(vporof)

FWIW, with the current aboutconfig.html table-based UI I get slight painting jank even if I filter to ~120 rows and hit "End" and then "Home" (to scroll to the extremes).

e.g. filtering for "layout" in the about:config searchbox gives me ~110-120 rows (6 viewports' worth of scrollable content), and it still produces 128ms rasterize times:
http://bit.ly/2WAdaXd

However, filtering for "gfx" gives me maybe 70-80 rows (4 viewports' worth), and it only gives me 25ms rasterize times:
http://bit.ly/2WuI8je

(In reply to Daniel Holbert [:dholbert] from comment #41)

FWIW, with the current aboutconfig.html table-based UI I get slight painting jank even if I filter to ~120 rows and hit "End" and then "Home" (to scroll to the extremes).

e.g. filtering for "layout" in the about:config searchbox gives me ~110-120 rows (6 viewports' worth of scrollable content), and it still produces 128ms rasterize times:
http://bit.ly/2WAdaXd

However, filtering for "gfx" gives me maybe 70-80 rows (4 viewports' worth), and it only gives me 25ms rasterize times:
http://bit.ly/2WuI8je

I've also made some plain HTML versions of the table for testing with no CSS, no JS, and running in the content process at:

(In reply to Florian Quèze [:florian] from comment #30)

  • There's A LOT of time spent removing DOM nodes when setting textContent to an empty string to clear the view. There must be a more efficient way to do this, and if there isn't we should talk to DOM people to see if they can help.

This looks like it might be bug 1480477 that I've uncovered a while back. Emilio are you still looking into that? Solving that might magically make this new about:config page significantly faster :)

Flags: needinfo?(emilio)

Yeah, looking at that profile it looks a lot like that. I'm not actively working on it ATM, though it's something I do want to fix. Time, though :(

Ways to not be blocked on me would be:

  • Instead of setting textContent to the empty string, just removing from the end.
  • Remove the parent instead?
Flags: needinfo?(emilio)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #44)

Yeah, looking at that profile it looks a lot like that. I'm not actively working on it ATM, though it's something I do want to fix. Time, though :(

Ways to not be blocked on me would be:

Oh, and of course not using :nth-child and co would also work in this case. Slightly annoying but if you're clearing and recomputing the items in the table it might not be a big deal anyway.

only ... Places

There are a number of others that have 100s or 1000s of entries, need sorting etc.. Cookies comes to mind, passwords, there are probably others.

Given these present requirements in Firefox, the need to fully replicate existing XUL tree behaviour in a single widget is not vital [for Firefox]

I agree, and stated so in comment 34. For most use cases, you do not need a full XUL <tree> with all its features. That's true. But you do need subsets of it, which cannot be done in plain HTML <table>. Naive attempts without a proper widget will fail.

For background: I say this with 20 years development experience with XUL and HTML, in a great number of projects. I've tried pretty much all the alternatives that you list, in actual code. None are suitable for anything that needs more than say 100 entries.

Places, about:config, cookies, passwords etc. all need more than what we have right now in HTML or <richlistbox>.

[ OFFTOPIC:

The decision to only consider Firefox use-cases moving forward when supporting internal widgets (and this includes XUL trees) has already been made

That doesn't make the decision right. That would be killing the central communication tool of 20 million people.

Killing XUL <tree> will outright kill Thunderbird. Once XUL tree is removed without a replacement that can do the same thing that XUL <tree> can, Thunderbird will be dead. Thunderbird depends on <tree> front and center (literally), and Thunderbird cannot fork Gecko, and it must keep secure, so TB will be left with no options and be dead. That discussion is clearly outside scope here, but Thunderbird continues to grow in users and I do not think it's worth killing a project like that over "cutting cruft we don't need". So, I think that consideration needs to be part of any <tree> plan.

To continue this meta-conversation, I suggest discussing the general aspects of XUL tree removal and planning
in an appropriate channel

Yes. Please tell me a means how this discussion can be led in constructive way and can come to an outcome that works.
Note that a discussion that starts or ends with "The decision has already been made" is not very constructive.
]

(In reply to Brian Grinstead [:bgrins] from comment #38)

I'd also like to get some measurements on this page when it's running in the chrome-priv content process instead of the parent process. Mike, is that something that could already be wired up locally, or will we have to wait on some more blockers on Bug 1513045 to be resolved first?

This can be wired up locally. Add this to the "config" entry in browser/components/about/AboutRedirector.cpp:

nsIAboutModule::URI_MUST_LOAD_IN_CHILD |
nsIAboutModule::URI_CAN_LOAD_IN_PRIVILEGED_CHILD

and then remove this:

https://searchfox.org/mozilla-central/rev/6e3cc153566f5f288ae768a2172385b8436d61dd/browser/components/aboutconfig/content/aboutconfig.js#318-319

build, and you should have about:config loading in the privileged content process.

Flags: needinfo?(mconley)

(In reply to Ben Bucksch (:BenB) from comment #46)

[ OFFTOPIC:

The decision to only consider Firefox use-cases moving forward when supporting internal widgets (and this includes XUL trees) has already been made

That doesn't make the decision right. That would be killing the central communication tool of 20 million people.

Killing XUL <tree> will outright kill Thunderbird. Once XUL tree is removed without a replacement that can do the same thing that XUL <tree> can, Thunderbird will be dead. Thunderbird depends on <tree> front and center (literally), and Thunderbird cannot fork Gecko, and it must keep secure, so TB will be left with no options and be dead. That discussion is clearly outside scope here, but Thunderbird continues to grow in users and I do not think it's worth killing a project like that over "cutting cruft we don't need". So, I think that consideration needs to be part of any <tree> plan.

Victor mentioned this, and I also did in Comment 33: this isn't happening right now. We are currently replacing the XBL bits of tree with Custom Elements, so they'll continue to work after XBL is gone. There also isn't a timeline on if/when the existing Places UI will be redone, and as long as it's around, XUL trees will continue to work. A shorter term need for TB to keep working on an unforked Gecko is the de-XBL effort (see Bug 1484976).

We are currently replacing the XBL bits of tree with Custom Elements, so they'll continue to work after XBL is gone

I did not think that XUL <tree> would survive the de-XULification, but if there's hope for that, that's great. That would be good news.

If I may humbly suggest an implementation strategy: To try to re-implement XUL <tree> with pure HTML, including virtualization and everything. Not everything at once, but the parts needed for projects like this. E.g. here the nesting is not needed, but columns are. So, for this project, a subset of the XUL <tree> could be implemented, but with the same API. If more features are needed for other use cases, they can be added there.

That would avoid complete re-write efforts like this here, because the old code could be mostly kept, and be lifted into HTML world. You would also have a direct comparison in features and performance. If you find things cannot be implemented, that would be a start to augment the HTML5 standard.

I think XUL was a reasonably sane API for implementing UIs, much more so than HTML. I think bringing XUL into HTML world is a worthwhile effort. (I specifically exclude XBL here, which was a mistake.)

This discussion has gone so far off the rails that I'm going to turn this into a metabug and move it back to Toolkit. The Graphics-specific portion of this is basically the same as bug 1456638 so we'll use that to track the graphics improvements. It looks like there are other improvements (e.g. the DOM stuff in bug 1480477) that could also help so I'm making those two bugs dependencies.

I'm also going to copy over the qf flag and some of the profile links to bug 1456638 so that we don't lose that state/information.

Component: Graphics → Preferences
Depends on: 1480477, 1456638
Product: Core → Toolkit
Summary: Scrolling aboutconfig.html checkerboards badly (janking when rasterizing table) → [meta] Scrolling aboutconfig.html checkerboards badly (janking when rasterizing table)

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #50)

I'm going to turn this into a metabug and move it back to Toolkit. The Graphics-specific portion of this is basically the same as bug 1456638 so we'll use that to track the graphics improvements. It looks like there are other improvements (e.g. the DOM stuff in bug 1480477) that could also help so I'm making those two bugs dependencies.

Thanks! I'll also file new dependencies based on the actionable items that emerged here.

Thank you to those who have provided feedback by analyzing the performance profiles!

Summary: [meta] Scrolling aboutconfig.html checkerboards badly (janking when rasterizing table) → [meta] Performance of "about:config"
Depends on: 1522671
Depends on: 1524786
Depends on: 1524787

(In reply to Brian Grinstead [:bgrins] from comment #36)

I copy/pasted the markup for the prefs table from the UI but with no JS / CSS into https://satin-dime.glitch.me/. Jamie, I'm curious if you are seeing the 15 second freeze with that page as well? Just trying to narrow down if it's related to the amount of content in the table, or if it's something particular with the JS / CSS in the new about:config page.

That takes over 1 min 20 sec to be remotely usable on my machine with Firefox + NVDA. I'd say a lot of that would be related to e10s Windows a11y performance issues. This means we'll be hit by that if about:config ever moves to a content process.

Flags: needinfo?(jteh)

(In reply to James Teh [:Jamie] from comment #52)

That takes over 1 min 20 sec to be remotely usable on my machine with Firefox + NVDA. I'd say a lot of that would be related to e10s Windows a11y performance issues. This means we'll be hit by that if about:config ever moves to a content process.

Sounds like we have few other options than introducing paging.
I think that would be the easiest and safest approach, since we can limit the amount of displayed rows to 100 or so.

Attached patch alternate-layout.diff (obsolete) (deleted) — Splinter Review

I have no idea if this will improve perf, but I played with this layout locally.

The layout from attachment 9041726 [details] [diff] [review] restricts the painting only to the table contents when scrolling, whereas without the patch applied, the painting is also done outside of the table area when scrolling. (I tested this with the paint flashing tool). Might be an improvement ?

Flags: needinfo?(mconley)
Flags: needinfo?(dholbert)

Tim, please create a new bug for this patch (and you should try phab :)

Comment on attachment 9041726 [details] [diff] [review] alternate-layout.diff Yes, this is a meta-bug. The idea seems worth trying, a separate bug would be great. Just ensure that the full page scrolling still behaves as it does now, and scrollbars are placed properly.
Attachment #9041726 - Attachment is obsolete: true
Flags: needinfo?(mconley)
Flags: needinfo?(dholbert)

This is a pretty long bug, and it's kind of meandered a bit, but I want to center in on something that florian wrote in comment 30:

The common reason for me to look at the whole list of preferences is to look for preferences that might have been affected by malware. I think this is a common case for people using about:config to troubleshoot misbehaving instances of Firefox on other people's computers. We could significantly improve the performance for this use case by showing all the modified preferences by default. That would be a short enough list to have it displayed quickly even without fixing the other performance issues. I'm not saying we should do this by the way, just giving an example of how changing the default behavior could affect the perception of performance without fixing the actual technical issues.

I think this is exactly how we need to angle our thinking. What are the actual use cases for about:config that we want to make sure we're optimized for?

This bug was originally opened because, I believe, it was noticed that if one were to rapidly scroll the full about:config list up and down, we'd drop frames. If that's so, that's a great stress test, but not really what I'd consider a valid use case.

The other thing I want us all to keep in mind (and which I definitely didn't make clear in comment 12), is that about:config is very much an internal page. There's lots and lots of work to make Firefox faster on web pages and use cases that millions and millions of users hit every day. I think I can state uncontroversially that optimizing every aspect of about:config is not the best use of our engineering time at this point.

So there's a balance we have to strike. We need to define the use cases that we need to optimize for and ensure we're sufficient for those cases. Anything else is scope creep.

(In reply to Mike Conley (:mconley) (:⚙️) from comment #58)

I think this is exactly how we need to angle our thinking. What are the actual use cases for about:config that we want to make sure we're optimized for?
The other thing I want us all to keep in mind (and which I definitely didn't make clear in comment 12), is that about:config is very much an internal page. [...] I think I can state uncontroversially that optimizing every aspect of about:config is not the best use of our engineering time at this point.

Thanks Mike, I'm glad to see we're on the same page here.

In fact I had bug 1524789 on file for the use case that Florian mentioned, and filed bug 1524787 to optimize the use case of typing. Since those are on file as separate bugs, it's easier to evaluate whether they make sense individually. For example, even if I have a patch up on the latter bug, it may even end up being too much complexity compared to the optimization we need, and we can discuss the benefits there.

I had a quick look at this from a graphics perspective.

It looks like we're painting a lot more pixels than we need to, and we could do a lot better by changing the page styles, or possibly by implementing complex optimizations in our painting code.

We're painting the whole page with not-quite-white (rgba 249,249,250,255) as the page canvas background color. Then we paint the entire table area (which covers very very nearly all the not-quite-white pixels) with actual white (due to --in-content-box-background being set as the background color for the table.

Finally we paint every second row in the table with a transparent color (rgba 12,12,13,0.05 - --in-content-box-background-odd) despite knowing that we'll be blended on top of opaque white and the opacity could be resolved in advance.

Switching the table background color to transparent (which changes the even row color to not-quite-white, but isn't noticeable), and making the odd rows use an opaque color (#EEE looks roughly right, but I can probably compute exactly what the existing color resolves to) results in a significant performance improvement for me.

We're also not using OMTP for painting in the parent process, so all this slowness blocks the main-thread. We could probably have that if we really needed it, or moving about:config to the privileged content process would also work.

For optimizing this painting internally, we'd need to detect that the table background obscures most of the page background. We do have code for this, but it a) uses rectangles, not regions (to reduce computational complexity), and the page has strips of visible content on all sides of the table, and b) doesn't handle rounded corners (which the table uses).

Depends on: 1527184
Depends on: 1527201
No longer depends on: 1524786
Depends on: 1529088

[Tracking Requested - why for this release]:

This is a "nightly-only" feature (bug 1529088), so can we probably remove the 67 tracking from this one

Priority: -- → P3

Bulk change for all regression bugs with status-firefox67 as 'fix-optional' to be marked 'affected' for status-firefox68.

Type: defect → task
Type: task → defect
Type: defect → task
Type: task → defect

Tim, if it is a regression, it is a defect on our product :)
you can remove the regression keyword if you want

It actually is a regression, and therefore a bug, so thanks Sylvestre.

Happy to take a patch for 70 but since this is triaged and set to P3 priority I'm setting this as fix-optional.
That will remove it from weekly regression triage.
The dependent bugs can be triaged separately.

Depends on: 1582535

Regarding performance, mobile has an HTML implementation of about:config showing all preferences by default and performs well using a dynamically appending items as you scroll.

https://searchfox.org/mozilla-central/rev/60c4067b1cbb0f94d7dc2d7cdfa27ed579817fee/mobile/android/chrome/geckoview/config.xhtml

Performance Impact: --- → P2
Whiteboard: [qf:p2:responsiveness]
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: