Build stylo on Android as default

RESOLVED FIXED in Firefox 58

Status

()

Firefox for Android
Build Config & IDE Support
RESOLVED FIXED
28 days ago
5 days ago

People

(Reporter: m_kato, Assigned: m_kato)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 58
Unspecified
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox56 unaffected, firefox57 wontfix, firefox58 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

28 days ago
Fennec team approves this changes, let's build stylo as default.  But preferece is still turned off.
(Assignee)

Comment 1

27 days ago
increase is 1.6MB (1,616,842 bytes) with the yesterday's mozilla-central.

Before: 36,686,341 bytes
After:  38,303,183 bytes
Comment hidden (mozreview-request)

Comment 3

27 days ago
mozreview-review
Comment on attachment 8922318 [details]
Bug 1411802 - Build stylo on Android as default.

https://reviewboard.mozilla.org/r/193370/#review198580
Attachment #8922318 - Flags: review?(snorp) → review+
Are there any Stylo on Android tests that will run in CI?  Or is the assumption that testing Stylo on Linux desktop is good enough for Android too?
Flags: needinfo?(m_kato)
Comment on attachment 8922318 [details]
Bug 1411802 - Build stylo on Android as default.

https://reviewboard.mozilla.org/r/193370/#review198622

I have questions, but nothing blocking.  I think 1.6Mb is a _huge_ hit to take when this finally rides to Release, but I guess that's what we pay for new technology.  Roll on!

::: taskcluster/ci/build/android.yml:28
(Diff revision 1)
>          secrets: true
>          custom-build-variant-cfg: api-16-debug
>          tooltool-downloads: internal
>      toolchains:
>          - android-sdk-linux
> +        - linux64-clang

Does clang provide the llvm backend for Rust?  I'm confused as to why Stylo, which is Rust, would require a _different_ host C/C++ compiler than we actually build with.  (What we actually build with is the Android NDK, as installed by tooltool.)

::: toolkit/moz.configure:578
(Diff revision 1)
> -@depends('--enable-stylo', '--help', target)
> -def stylo_config(value, _, target):
> +@depends('--enable-stylo', '--help', target, milestone)
> +def stylo_config(value, _, target, milestone):
>      build_stylo = None
>      enable_stylo = None
>  
>      # If nothing is specified, default to building and enabling Stylo where it

This comment isn't accurate anymore.  (Assuming that Stylo works on Android.)

::: toolkit/moz.configure:583
(Diff revision 1)
>      # If nothing is specified, default to building and enabling Stylo where it
>      # is known to work.
>      if value.origin == 'default':
>          if target.os == 'Android':
> -            # Stylo on Android is happening Later(tm).
> -            pass
> +            if milestone.is_nightly:
> +                build_stylo = True

Consider adding `enable_stylo = False` for clarity.
Attachment #8922318 - Flags: review?(nalexander) → review+
Comment on attachment 8922318 [details]
Bug 1411802 - Build stylo on Android as default.

https://reviewboard.mozilla.org/r/193370/#review198622

Oh!  Please ensure that this doesn't break artifact builds, on ARM and x86.  Is there any aarch64 implication for this setting?
Comment on attachment 8922318 [details]
Bug 1411802 - Build stylo on Android as default.

https://reviewboard.mozilla.org/r/193370/#review198632

::: taskcluster/ci/build/android.yml:28
(Diff revision 1)
>          secrets: true
>          custom-build-variant-cfg: api-16-debug
>          tooltool-downloads: internal
>      toolchains:
>          - android-sdk-linux
> +        - linux64-clang

I think this is about Stylo's use of bindgen to generate Rust wrappers for C++ code.  bindgen uses clang to do this work.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> Comment on attachment 8922318 [details]
> Bug 1411802 - Build stylo on Android as default.
> 
> https://reviewboard.mozilla.org/r/193370/#review198632
> 
> ::: taskcluster/ci/build/android.yml:28
> (Diff revision 1)
> >          secrets: true
> >          custom-build-variant-cfg: api-16-debug
> >          tooltool-downloads: internal
> >      toolchains:
> >          - android-sdk-linux
> > +        - linux64-clang
> 
> I think this is about Stylo's use of bindgen to generate Rust wrappers for
> C++ code.  bindgen uses clang to do this work.

OK, thanks.
Comment on attachment 8922318 [details]
Bug 1411802 - Build stylo on Android as default.

https://reviewboard.mozilla.org/r/193370/#review198650

Looks good to me.  I assume there's some CI plan one way or another, so no reason to block this patch on it.
Attachment #8922318 - Flags: review?(jryans) → review+
(In reply to Nick Alexander :nalexander from comment #5)

> I think 1.6Mb is a _huge_ hit to
> take when this finally rides to Release, but I guess that's what we pay for
> new technology.  Roll on!

Indeed, presumably some of this is first-mover cost for some Rust dependencies. The more Rust code we ship — and we will ship more! — the more this cost will be amortized over components.

I've always opposed increasing APK sizes, but in this case I am very happy to be aggressive: the more eagerly we pay these costs, the less resistance will apply to all subsequent uses of Rust, and the more we can leverage the technology to make our products better.
(In reply to Nick Alexander :nalexander from comment #5)
> I have questions, but nothing blocking.  I think 1.6Mb is a _huge_ hit to
> take when this finally rides to Release, but I guess that's what we pay for
> new technology.

The APK size increase will be smaller when we actually ship Stylo on Android because we can remove Gecko's old style system code at that time. During the transition to enabling Stylo by default, we must ship both the old and new style system code.
status-firefox56: --- → unaffected
status-firefox57: --- → wontfix
status-firefox-esr52: --- → unaffected
(In reply to Richard Newman [:rnewman] from comment #10)
> (In reply to Nick Alexander :nalexander from comment #5)
> 
> > I think 1.6Mb is a _huge_ hit to
> > take when this finally rides to Release, but I guess that's what we pay for
> > new technology.  Roll on!
> 
> Indeed, presumably some of this is first-mover cost for some Rust
> dependencies. The more Rust code we ship — and we will ship more! — the more
> this cost will be amortized over components.
> 
> I've always opposed increasing APK sizes, but in this case I am very happy
> to be aggressive: the more eagerly we pay these costs, the less resistance
> will apply to all subsequent uses of Rust, and the more we can leverage the
> technology to make our products better.

As someone who has died on this hill once before, I also agree that the size increase is warranted here.
(In reply to Chris Peterson [:cpeterson] from comment #11)
> (In reply to Nick Alexander :nalexander from comment #5)
> > I have questions, but nothing blocking.  I think 1.6Mb is a _huge_ hit to
> > take when this finally rides to Release, but I guess that's what we pay for
> > new technology.
> 
> The APK size increase will be smaller when we actually ship Stylo on Android
> because we can remove Gecko's old style system code at that time. During the
> transition to enabling Stylo by default, we must ship both the old and new
> style system code.

Speaking of which, maybe it's time to have a build option to disable the gecko style system (and check how big the apk is without it)
(In reply to Mike Hommey [:glandium] from comment #13)
> Speaking of which, maybe it's time to have a build option to disable the
> gecko style system (and check how big the apk is without it)

bholley didn't think adding a build option was good use of time because we plan to remove the Gecko style system code soon (in ~59) when we ship Stylo support for Android and chrome/XUL. He estimated that removing the Gecko code might save about 700 KB.
(Assignee)

Comment 15

27 days ago
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #4)
> Are there any Stylo on Android tests that will run in CI?  Or is the
> assumption that testing Stylo on Linux desktop is good enough for Android
> too?

Some tests is run on Android only.  So After landing this, I will add tier 3 jobs such as styloVsGecko (bug 1406290 etc)
Flags: needinfo?(m_kato)

Comment 16

24 days ago
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1c3b82079a5
Build stylo on Android as default. r=nalexander,snorp,jryans
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e4f10cea5e7
Fix bustage after updating NDK r15c. r=me
https://hg.mozilla.org/mozilla-central/rev/c1c3b82079a5
https://hg.mozilla.org/mozilla-central/rev/1e4f10cea5e7
Status: NEW → RESOLVED
Last Resolved: 24 days ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Fennec Nightly 58 20171030100130 @ Android 7.0 (Moto G5)
Verified: It's possible to enable stylo and stylo-chrome and they work.
Depends on: 1413041

Updated

5 days ago
QA Contact: ioana.chiorean
You need to log in before you can comment on or make changes to this bug.