Closed Bug 1470983 Opened 3 years ago Closed 4 months ago

Process LookAndFeel data in the parent process

Categories

(Core :: Widget: Gtk, enhancement)

All
Linux
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
86 Branch
Tracking Status
firefox86 --- fixed

People

(Reporter: kmag, Assigned: heycam)

References

(Blocks 3 open bugs, Regression)

Details

(Whiteboard: [overhead:>900k][qf-])

Attachments

(2 files, 1 obsolete file)

On Linux, loading the GTK style system to setup LookAndFeel currently allocates over 900K of unreported per process. I'm not sure of the numbers for other platforms. I suspect it's less on most of them, given GTK CSS style system, but I suspect it's still non-trivial.

It would be nice to load this data in the parent and share the values we need with the content process. Even better if we can do that as shared memory snapshots.

This should also, in theory, help content startup perf, since creating a bunch of widgets to calculate their look-and-feel properties is pretty expensive.
Whiteboard: [overhead:>900k][qf] → [overhead:>900k][qf-]
This is one of the larger sources of per content process overhead, Jim do you know who can look at this? It seems like we might want to redirect to the layout team.
Flags: needinfo?(jmathies)
Component: Widget → Widget: Gtk
Flags: needinfo?(jmathies)
OS: Unspecified → Linux
Hardware: Unspecified → All
Flags: needinfo?(jmathies)
(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #1)
> This is one of the larger sources of per content process overhead, Jim do
> you know who can look at this? It seems like we might want to redirect to
> the layout team.

Hmm, I don't have anyone with the cycles for this currently. We could ask the redhat engineers if they can help out. Besides that, I'm not sure who to turn to. We really don;t have any owners for gtk right now.
Flags: needinfo?(jmathies)
Note that we'd need to do something for drawing also, or GTK will still load this data.

This seems to be another reason for bug 1411425.
Depends on: linux-nnt

We'll want some way to ship the theme info to the child process for bug 1129492. It's possible to run the content processes in headless mode and never initialize GTK (bug 1640345), but that uses HeadlessLookAndFeel which doesn't match the browser chrome. (Text selection color is the main difference I've noticed, but there are probably others.)

Blocks: 1129492
See Also: → 1640345

I have a prototype of this that seems to at least partially work.

Assignee: nobody → jld
See Also: → 1503054

The GTK nsLookAndFeel implementation uses gtk_widget_get_settings
on temporary widgets that aren't attached to anything, which the
documentation says not to do. Empirically, this seems to return the
same settings object as gtk_settings_get_default.

Explicitly using gtk_settings_get_default is useful for remote
look-and-feel, so that the parent process can register for property
changes on that object to know when to update the child processes.

TODO:

  • Comments!
    ** There are subtle ordering things in content process startup.
    ** Also the "compressed" representation is a little odd.
  • Testing??
    ** Non-default settings? Which ones?
    ** Dynamic changes (how does that work?)
  • Pref off on non-(GTK and Nightly) or something

(In reply to Jed Davis [:jld] ⟨⏰|UTC-6⟩ ⟦he/him⟧ from comment #7)

** Also the "compressed" representation is a little odd.

I was curious how many L&F values we tend to have present. Testing showed the following, on a new profile:

Windows: Int 49/65, Color  84/121
macOS:   Int 42/65, Color 101/121
Linux:   Int 47/65, Color  88/121

Windows is using the least -- that's 53 values of 32 bits each = 212 bytes we're saving in values by using the representation in the patch. That doesn't seem that significant, so it seems like the simplicity of having all int and color values taking space, and a BitSet indicating whether one is present, might be worth the extra space.

I didn't test floats (there are only three) or fonts (which I think should be present on all platforms).

I'm also collapsing identical values. I think my main concern there was the font info, where we have 18 selectors but the GTK backend maps that down to at most 4 distinct values, and the font info contains an nsString.

Empirically, on Linux:

  • 14/65 distinct ints
  • 23/120 distinct colors (more than I expected)
  • 2/3 distinct floats
  • 1/17 distinct fonts (name length: 6)

Given that sizeof(LookAndFeelFont) == 40, the 8-byte string header + 6 UTF-16 code units rounds up to a 32-byte allocation, each compressed element still takes 1 byte, and the 4 extra nsTArray overheads (just counting the pointer and header, not trying to reason about malloc overhead here):

(rr) p (65-14)*3 + (120-23)*3 + 1*3 + 16*(39+32) - 4*16
$23 = 1519

Windows is similar:

  • 17/65 distinct ints
  • 21/120 distinct colors
  • 1/3 distinct floats
  • 2/17 distinct fonts (name lengths: 8 (x11), 14 (x4))

So, ~1.5kB per process, maybe a little more if the fonts have longer names. Not nothing in the post-Fission world, but maybe not worth the complexity / code size increase after all.

OK, in my initial reading I overlooked the fact that it's storing only unique values. I'm going to leave it in.

Taking Jed's patch to polish up.

Assignee: jld → cam
Depends on: 1678540

(In reply to Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ from comment #7)

** There are subtle ordering things in content process startup.

What this was referring to is how ContentChild::RecvSetXPCOMProcessAttributes happens before the LookAndFeel is instantiated (or at least I hope it's a happens-before relationship). The existing “cache” path stores the data into a global variable, and then the LookAndFeel picks it up when it's constructed. The path I added is a little different: constructs the RemoteLookAndFeel object at RecvSetXPCOM… time, stores that in a different global, and then later when the LookAndFeel would normally be constructed, the existing RemoteLookAndFeel is used instead. In hindsight that might be unnecessary complication; I wasn't familiar with this area of the code when I started working on it.

Thanks for clarifying that comment. I'll try to make the RemoteLookAndFeel instantiation more similar to the LNF cache path.

I'm not really sure what to do with automated testing, as we don't have a way to change Gtk settings to test our response to them. (Well, I guess we could add some test-only C++ functions that call g_object_set(GtkSettings*, ...) to do this.) I've tested manually for both Gtk theme changes and other settings (like cursor blink speed), and they work.

This adds a new LookAndFeel implementation, RemoteLookAndFeel, which can
be used in content processes and is supplied with all of its values by the
parent process.

Co-authored-by: Cameron McCormack <cam@mcc.id.au>

Attachment #9183359 - Attachment is obsolete: true
Blocks: 1672097
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1fcf791ba732
Prelude: Remove use of gtk_widget_get_settings in LookAndFeel. r=heycam
https://hg.mozilla.org/integration/autoland/rev/2193d5d258b4
Remote all LookAndFeel values for the Gtk backend. r=spohl,jld
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → 86 Branch
Regressions: 1683026
Regressed by: 1683345

(In reply to Kris Maglione [:kmag] from comment #0)

On Linux, loading the GTK style system to setup LookAndFeel currently
allocates over 900K of unreported per process. I'm not sure of the numbers
for other platforms. I suspect it's less on most of them, given GTK CSS
style system, but I suspect it's still non-trivial.

It would be nice to load this data in the parent and share the values we
need with the content process. Even better if we can do that as shared
memory snapshots.

We didn't in the end use a shared memory snapshot, so there is still some per-content-process memory usage for the LookAndFeel tables. Per comment 9 it's around 1.5 KiB per process.

https://bugzilla.mozilla.org/show_bug.cgi?id=1680175#c4 reports that we saved around 400 KiB per content process with this bug's patches. I guess it'll be more once we don't initialize Gtk at all and switch to using the non-native theme.

This should also, in theory, help content startup perf, since creating a
bunch of widgets to calculate their look-and-feel properties is pretty
expensive.

Looks like it reduced content process startup time by 12-14% / 16 ms on the CI machines.

You need to log in before you can comment on or make changes to this bug.