Remove XUL dependency in b2g

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: fabrice, Assigned: fabrice)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P1])

Attachments

(3 attachments, 7 obsolete attachments)

(Assignee)

Description

5 years ago
We don't really use xul in b2g (except in the <video> and <audio> bindings) but we still build everything and just using a xul window probably pages in some of this code. Let's try to get rid of what we can.
(Assignee)

Updated

5 years ago
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → All
(Assignee)

Comment 1

5 years ago
Created attachment 777142 [details] [diff] [review]
Use xhtml instead of xul for b2g main window

That works fine for me, and the memory usage seems to go down a bit:

xhtml:

fabrice@fabrice-x230:~/dev/b2g/B2G$ adb shell b2g-info
                          |     megabytes    |
           NAME  PID NICE  USS  PSS  RSS VSIZE OOM_ADJ USER    
            b2g 1510    0 47.7 51.1 61.4 175.9       0 root    
          Usage 1564   18 10.9 14.0 24.1  65.2       6 app_1564
     Homescreen 1569   18 13.1 16.5 26.8  67.9       4 app_1569
(Preallocated a 1640   18  9.5 12.1 21.2  62.7       6 root    

System memory info:

            Total 397.2 MB
     Used - cache 100.1 MB
  B2G procs (PSS)  93.8 MB
    Non-B2G procs   6.3 MB
     Free + cache 297.2 MB
             Free 186.8 MB
            Cache 110.4 MB

=====================================================================
xul:

fabrice@fabrice-x230:~/dev/b2g/B2G$ adb shell b2g-info
                          |     megabytes    |
           NAME  PID NICE  USS  PSS  RSS VSIZE OOM_ADJ USER    
            b2g 1824    0 48.5 52.0 62.2 175.8       0 root    
          Usage 1877   18 10.9 14.0 24.1  65.2       6 app_1877
     Homescreen 1883   18 13.1 16.5 26.8  67.9       4 app_1883
(Preallocated a 1952   18  9.5 12.1 21.2  62.7       6 root    

System memory info:

            Total 397.2 MB
     Used - cache 102.1 MB
  B2G procs (PSS)  94.6 MB
    Non-B2G procs   7.5 MB
     Free + cache 295.2 MB
             Free 184.0 MB
            Cache 111.2 MB
Assignee: nobody → fabrice
Attachment #777142 - Flags: review?(21)
(Assignee)

Comment 2

5 years ago
Created attachment 777143 [details] [diff] [review]
wip: building with --disable-xul

That doesn't compile yet... :

In file included from ../../dist/include/mozilla/dom/BindingDeclarations.h:23:0,
                 from /home/fabrice/dev/builds/obj-b2g-desktop-birch/dom/bindings/DocumentBinding.h:7,
                 from /home/fabrice/dev/builds/obj-b2g-desktop-birch/dom/bindings/HTMLAppletElementBinding.cpp:4:
../../dist/include/nsAutoPtr.h: In instantiation of ‘nsRefPtr<T>& nsRefPtr<T>::operator=(const already_AddRefed<U>&) [with I = nsFrameLoader; T = nsIFrameLoader]’:
/home/fabrice/dev/builds/obj-b2g-desktop-birch/dom/bindings/HTMLAppletElementBinding.cpp:611:33:   required from here
../../dist/include/nsAutoPtr.h:945:11: error: no matching function for call to ‘nsRefPtr<nsIFrameLoader>::assign_assuming_AddRef(nsFrameLoader* const&)’
../../dist/include/nsAutoPtr.h:945:11: note: candidate is:
../../dist/include/nsAutoPtr.h:863:7: note: void nsRefPtr<T>::assign_assuming_AddRef(T*) [with T = nsIFrameLoader]
../../dist/include/nsAutoPtr.h:863:7: note:   no known conversion for argument 1 from ‘nsFrameLoader* const’ to ‘nsIFrameLoader*’

In the directory  /home/fabrice/dev/builds/obj-b2g-desktop-birch/dom/bindings
(Assignee)

Comment 3

5 years ago
Justin, do you think the gain from comment 1 could justify an uplift for 1.1?
(In reply to Fabrice Desré [:fabrice] from comment #3)
> Justin, do you think the gain from comment 1 could justify an uplift for 1.1?

Probably not?  3mb (the difference in "free+cache") is a serious win for 256mb devices, but 1.1 is targeted at 512mb devices, and the 256mb devices, if they get 1.1, are incidental, aiui.
Whiteboard: [MemShrink]
(Assignee)

Comment 5

5 years ago
Current 1.0.1 devices will get updates to 1.1 and have only 256mb.
(In reply to Fabrice Desré [:fabrice] from comment #5)
> Current 1.0.1 devices will get updates to 1.1 and have only 256mb.

A 3mb win is definitely significant for 256mb devices.

I don't know about the downsides of taking this patch, though.
Comment on attachment 777142 [details] [diff] [review]
Use xhtml instead of xul for b2g main window

Review of attachment 777142 [details] [diff] [review]:
-----------------------------------------------------------------

that's probably going to break some tests http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#931 that use overlays.
It would be nice to check if those still works or to provide a way for them to work (this patch will also break some Gaia extensions for developing).

I also wonder if the |content| property in shell.js works if the root is not a xul document - it worth checking that to not regress.

The rest of the code looks fine to me and will definitively sign the death of xul in B2G if we can get change the video/audio bindings.
Attachment #777142 - Flags: review?(21) → feedback+
Also can you do an about:memory to see if it is visible there?
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #8)
> Also can you do an about:memory to see if it is visible there?

about:memory is XML not XUL, but anyway we don't display it directly on the phone; we dump log files and pull them off the phone.  That shouldn't be broken by this change.
(Assignee)

Comment 10

5 years ago
(In reply to Justin Lebar [:jlebar] from comment #9)
> (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #8)
> > Also can you do an about:memory to see if it is visible there?
> 
> about:memory is XML not XUL, but anyway we don't display it directly on the
> phone; we dump log files and pull them off the phone.  That shouldn't be
> broken by this change.

I think Vivien wants to check that the memory gain we see in b2g-info also shows up in memory dumps.
(Assignee)

Comment 11

5 years ago
Created attachment 777868 [details]
memory-logs.zip

Here are 2 about:memory logs:
about-memory-0 is without the patch, about-memory-1 with the patch. There's an improvement in the explicit allocations in the parent process.
(Assignee)

Comment 12

5 years ago
Created attachment 778024 [details] [diff] [review]
Use xhtml instead of xul for b2g main window, v2

This patch is green on try: https://tbpl.mozilla.org/?tree=Try&rev=cacc1cc38672
Attachment #777142 - Attachment is obsolete: true
Attachment #778024 - Flags: review?
(In reply to Fabrice Desré [:fabrice] from comment #12)
> Created attachment 778024 [details] [diff] [review]
> Use xhtml instead of xul for b2g main window, v2
> 
> This patch is green on try:
> https://tbpl.mozilla.org/?tree=Try&rev=cacc1cc38672

For some reason try build don't run b2g tests (like mochitests or xpcshell).
(I think a test everything try run them)

We might break mochitest, but may be not. The mochitest file vivien pointed out may no longer be involved for b2g testing. Both xpcshell and mochitests,
have completely different codepath for b2g:
http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/runtestsb2g.py
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtestsb2g.py
As it looks more based on marionette, which doesn't seem to use overlay,
it should'nt be impacted by this change.

Otherwise, regarding extensions, I don't think it will break gaia helper addons either as we do not do anything with shell.xul. We copy pieces of shell.js and some other JS components but that's pretty much it.
It will certainly require some work for the simulator (which uses overlays), but nothing really important as the simulator is still based on 1.1.
Comment on attachment 778024 [details] [diff] [review]
Use xhtml instead of xul for b2g main window, v2

Review of attachment 778024 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/chrome/content/shell.xhtml
@@ +3,5 @@
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this file,
>     - You can obtain one at http://mozilla.org/MPL/2.0/.  -->
>  
> +<xhtml xmlns="http://www.w3.org/1999/xhtml"

You mean <html, not <xhtml.
(Assignee)

Comment 15

5 years ago
(In reply to Alexandre Poirot (:ochameau) from comment #13)

> As it looks more based on marionette, which doesn't seem to use overlay,
> it should'nt be impacted by this change.

Well, there is some test breakage to fix: https://tbpl.mozilla.org/?tree=Try&rev=ec6c501c57fb

> Otherwise, regarding extensions, I don't think it will break gaia helper
> addons either as we do not do anything with shell.xul. We copy pieces of
> shell.js and some other JS components but that's pretty much it.
> It will certainly require some work for the simulator (which uses overlays),
> but nothing really important as the simulator is still based on 1.1.

Yes, I'll sync up with Myk to make sure we have a solution for the simulator before landing anything.

(In reply to :Ms2ger from comment #14)

> 
> You mean <html, not <xhtml.

Ouch, yes. And this worked!
(Assignee)

Comment 16

5 years ago
Created attachment 778927 [details] [diff] [review]
Use xhtml instead of xul for b2g main window, v3

Some tests fixes later:
https://tbpl.mozilla.org/?tree=Try&rev=dcd05c668fa7

The only failure I don't understand is the change I had to make in test_element_touch.py - Without that, the test times out like if the button was not pressed.
Attachment #778024 - Attachment is obsolete: true
Attachment #778024 - Flags: review?
This sounds great!
Whiteboard: [MemShrink] → [MemShrink:P1]
(Assignee)

Comment 18

5 years ago
Created attachment 781747 [details] [diff] [review]
wip: building with --disable-xul
Attachment #777143 - Attachment is obsolete: true
(Assignee)

Comment 19

5 years ago
Created attachment 787821 [details] [diff] [review]
Use html instead of xul for b2g main window, v4

Updated patch since parts of the b2g test harness have changed. Try build is at https://tbpl.mozilla.org/?tree=Try&rev=b97ae2e3caca and looks ok.

I also have a patch that let us build with --disable-xul but I think it's out of scope for this bug. We get not much savings with it, and that breaks a few things we care about and that would need to be reimplemented (video controls, some accessibility code, the html menu element...).

I checked with the simulator guys and they are ok to land this now.
Attachment #777868 - Attachment is obsolete: true
Attachment #778927 - Attachment is obsolete: true
Attachment #781747 - Attachment is obsolete: true
Attachment #787821 - Flags: review?(21)
(Assignee)

Comment 20

5 years ago
Created attachment 787825 [details]
baseline memory report
(Assignee)

Comment 21

5 years ago
Created attachment 787827 [details]
no xul window memory report

Using the 'diff' functionnality of about:memory shows a 1.36MB win in the parent process explicit allocation with this patch.
> a 1.36MB win in the parent process explicit allocation

\o/
Comment on attachment 787821 [details] [diff] [review]
Use html instead of xul for b2g main window, v4

I know people expect me to kill xul but since this xul is my secret garden of love I prefer to leave for vacation tomorrow and I will let this honor to Alex ;)

Alex make sure the extension overlay mechanism used for both the simulator and the b2g desktop build with DEBUG=1 works.
Attachment #787821 - Flags: review?(21) → review?(poirot.alex)
Comment on attachment 787821 [details] [diff] [review]
Use html instead of xul for b2g main window, v4

Review of attachment 787821 [details] [diff] [review]:
-----------------------------------------------------------------

The patch looks good codewise, but we need to fix these two issues:
* on device, it looks like there is some margin or padding on top (at least),
as icons on bottoms are cropped (whereas there should be some free pixels bottom them)
* on desktop, the toplevel window ends up being a very small window like 30x30

::: build/mobile/b2gautomation.py
@@ +277,5 @@
>          # run the script that starts the tests
>          if self.test_script:
>              if os.path.isfile(self.test_script):
>                  script = open(self.test_script, 'r')
> +                print "marionnette script: ", self.test_script

Is this a debug statement or a usefull new message?

::: testing/mochitest/runtests.py
@@ +401,5 @@
>      chrome = ""
>      if options.browserChrome or options.chrome or options.a11y or options.webapprtChrome:
>        chrome += """
>  overlay chrome://browser/content/browser.xul chrome://mochikit/content/browser-test-overlay.xul
> +overlay chrome://browser/content/shell.xhtml chrome://mochikit/content/browser-test-overlay.xul

I'm not expecting this to work. TBPL doesn't use runtests.py on b2g, but runtestsb2g.py, so fixing this doesn't look mandatory.
Having said that, I'm currently submitting a patch to make this script to work on b2g desktop, but I won't consider this as a blocker to land this.
Attachment #787821 - Flags: review?(poirot.alex) → review-
(Assignee)

Comment 25

5 years ago
Created attachment 795841 [details] [diff] [review]
Use html instead of xul for b2g main window, v5

(In reply to Alexandre Poirot (:ochameau) from comment #24)

> The patch looks good codewise, but we need to fix these two issues:
> * on device, it looks like there is some margin or padding on top (at least),
> as icons on bottoms are cropped (whereas there should be some free pixels
> bottom them)

I fixed that by inserting the main iframe in a <body> element.

> * on desktop, the toplevel window ends up being a very small window like
> 30x30

I fixed a mistake in screen.js, but even without this change it was ok for me on linux desktop. Which platform are you testing on?

> ::: build/mobile/b2gautomation.py
> @@ +277,5 @@
> >          # run the script that starts the tests
> >          if self.test_script:
> >              if os.path.isfile(self.test_script):
> >                  script = open(self.test_script, 'r')
> > +                print "marionnette script: ", self.test_script
> 
> Is this a debug statement or a usefull new message?

Debugging leftover, removed.

> ::: testing/mochitest/runtests.py
> @@ +401,5 @@
> >      chrome = ""
> >      if options.browserChrome or options.chrome or options.a11y or options.webapprtChrome:
> >        chrome += """
> >  overlay chrome://browser/content/browser.xul chrome://mochikit/content/browser-test-overlay.xul
> > +overlay chrome://browser/content/shell.xhtml chrome://mochikit/content/browser-test-overlay.xul
> 
> I'm not expecting this to work. TBPL doesn't use runtests.py on b2g, but
> runtestsb2g.py, so fixing this doesn't look mandatory.
> Having said that, I'm currently submitting a patch to make this script to
> work on b2g desktop, but I won't consider this as a blocker to land this.

I left my change because I'm not sure what you're asking for here, and shell.xul was obviously wrong now. But let me know if you want me eg. to just remove this line.
Attachment #787821 - Attachment is obsolete: true
Attachment #795841 - Flags: review?(poirot.alex)
(Assignee)

Comment 26

5 years ago
I re-did some memory usage measures and got that in the main process:

-0.38 MB (100.0%) -- explicit

-0.28 MB ── heap-allocated
-0.22 MB ── heap-committed
   0.80% ── heap-overhead-ratio
  24,732 ── page-faults-soft
-0.52 MB ── resident
-0.46 MB ── resident-unique
-2.05 MB ── vsize
Comment on attachment 795841 [details] [diff] [review]
Use html instead of xul for b2g main window, v5

Review of attachment 795841 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good on device and desktop (linux/ubuntu).

It clearly break the simulator usage, but I don't want to block on this.
I may submit a followup to make it work with html, if required.
Attachment #795841 - Flags: review?(poirot.alex) → review+
https://hg.mozilla.org/mozilla-central/rev/5c25126b9c76
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Blocks: 911751
Backed out for causing bug 911751.
https://hg.mozilla.org/integration/b2g-inbound/rev/3ec404b808b2
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 32

5 years ago
Now with a hack for Mac OS desktop users:
https://hg.mozilla.org/integration/b2g-inbound/rev/36880bc561d6
https://hg.mozilla.org/mozilla-central/rev/36880bc561d6
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.