Closed Bug 1342450 Opened 8 years ago Closed 8 years ago

Build (but don't enable) webrender by default on nightly builds

Categories

(Core :: Graphics: WebRender, defect, P3)

Product:
Core
Component:
Graphics: WebRender
Version:
54 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: kats, Assigned: kats)

References

(Depends on 1 open bug)

Details

(Whiteboard: [gfx-noted])

Attachments

(3 files)

Bug 1342450 - Rename MOZ_ENABLE_WEBRENDER to MOZ_BUILD_WEBRENDER.
59 bytes, text/x-review-board-request
rhunt
: review+
Details
Bug 1342450 - Extract a MOZ_ENABLE_WEBRENDER from MOZ_BUILD_WEBRENDER so that we build but disable by default.
59 bytes, text/x-review-board-request
rhunt
: review+
froydnj
: review+
ted
: review+
Details
Bug 1342450 - Keep webrender disabled by default on OS X buildbot builders.
59 bytes, text/x-review-board-request
ted
: review+
Details 
Right now:
1) webrender is not built by default
2) building webrender (the rust code) can be enabled by setting `ac_add_options --enable-webrender` in your mozconfig
3) at runtime, webrender is enabled by default on --enable-webrender builds, but can be disabled by explicitly setting the startup pref gfx.webrender.enabled to false
4) In automation, we have a "QuantumRender" platform which does builds and some tests with webrender enabled.

What we want to move to, is:
1) build webrender by default in nightly/local-developer builds (but not riding the trains yet)
2) allow developers to disable webrender from being built, by setting `ac_add_options --disable-webrender` in the mozconfig
3) at runtime, have webrender disabled by default (gfx.webrender.enabled=false)
4) Keep our automation level untouched.

Further to this, I think we have webrender enabled by default at runtime (gfx.webrender.enabled=true) if people explicitly set `ac_add_options --enable-webrender` in their mozconfig. This will provide seamless backwards compatibility in behaviour for graphics developers who are already building with this flag and expecting webrender to be enabled at runtime. It will also give us the desired behaviour in automation (maintaining the behaviour for QuantumRender builds).

So the logic we want is:
- have two defines, say MOZ_BUILD_WEBRENDER and MOZ_ENABLE_WEBRENDER
- MOZ_BUILD_WEBRENDER would be set by default on nightly/local builds, but can be unset by setting --disable-webrender
- MOZ_ENABLE_WEBRENDER would be unset by default on nightly/local builds, but can be set by setting --enable-webrender
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #0)
> Further to this, I think we have webrender enabled by default at runtime

s/have/want/
The buildbot OS X builds are failing with what appears to be a clang-internal linker error. The Taskcluster builds are fine. Buildbot seems to be using clang 3.8.0 while TC is using clang 3.9.0, so maybe upgrading buildbot will fix this?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3)
> The buildbot OS X builds are failing

There's some discussion in bug 1342503 where I investigated this. I think for now maybe it's easier to skip enabling webrender by default on this platform. We'll have to skip android as well since webrender doesn't build there yet. We'll still get Windows (our target platform) and Linux (the most useful platform) so I think it would be beneficial to do this regardless.
So I skipped buildbot OS X and Android, and did another try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcc2605203a77e52b7c6216e0ea7099eb9d77307

It shows:
1) a Linux x64 debug static analysis build failure
   - this is because the build is for some reason running on buildbot and has an old version of x11. If we migrate this to TC it should be running in a more modern environment and be happy. Following up on bug 1264494 for that.
2) a bunch of windows failures
   - What seems to be happening here is that there are assertions, and while trying to generate the stack trace for those assertions, the stack dumping code dies. That, in turn, is because it has bad symbol files coming in. I downloaded the symbol zip from [1] which is used in the tests, and it looks like the symbol file for libxul is truncated. The end of it looks like this:

 -----
 1a647e 13 1367 347518
 1a6491 5 1368 347518
 FUNC 1a64a0 5 0 core::sync::atomic::fence
 1a64a0 3 1454 347518
 1a64a3 0 1457 347518
 1a64a3 2 1465 347518
 FUNC 1a6
 -----

That last "FUNC" line appears truncated, and is what the parser is dying on (I think). Not sure yet why this file is truncated, but it's generated in the job at [2].

[1] https://queue.taskcluster.net/v1/task/TH_Po8RgR72VzFONT4skhg/artifacts/public/build/firefox-54.0a1.en-US.win32.crashreporter-symbols.zip
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcc2605203a77e52b7c6216e0ea7099eb9d77307&selectedJob=80875684
Depends on: 1264494
New try push, with the fix for bug 1343625 included: https://treeherder.mozilla.org/#/jobs?repo=try&revision=08fd35010f0b5d58def870e29b7b858f69cda0ff

The linux 64 debug static analysis build failure (on buildbot) is expected and can be ignored as that job will be removed entirely in bug 1344297.
The try push also shows a high incidence of Win 7 opt R(R) redness (4/9 so far). This appears to be a pre-existing issue which has been getting starred as tens of different bugs (see the dep list of bug 1345093) so I filed bug 1345093 for the root cause. I'm not sure if these patches make that failure more frequent or not, it's hard to tell what the incidence of it was previously because it's spread across so many different bugs.
Shoot, I just realized this will break local Android builds. Updated part 2/3 coming.
Comment on attachment 8846794 [details]
Bug 1342450 - Rename MOZ_ENABLE_WEBRENDER to MOZ_BUILD_WEBRENDER.

https://reviewboard.mozilla.org/r/119804/#review121732
Attachment #8846794 - Flags: review?(rhunt) → review+
I sent a post to dev-platform about this [1]. I also realized I did a better job explaining the changes there than I did in the commit messages, so I'll update those. Updated commit messages incoming, patches are functionally the same.

[1] https://groups.google.com/d/msg/mozilla.dev.platform/W0p1G31pZWs/l-hkstgQBgAJ
Comment on attachment 8846795 [details]
Bug 1342450 - Extract a MOZ_ENABLE_WEBRENDER from MOZ_BUILD_WEBRENDER so that we build but disable by default.

https://reviewboard.mozilla.org/r/119806/#review123042
Attachment #8846795 - Flags: review?(rhunt) → review+
^ Rebased to master, and s/ted/froydnj/ since ted has been pretty busy, and Nathan graciously accepted to do the reviews instead.
Comment on attachment 8846795 [details]
Bug 1342450 - Extract a MOZ_ENABLE_WEBRENDER from MOZ_BUILD_WEBRENDER so that we build but disable by default.

https://reviewboard.mozilla.org/r/119806/#review124924

I'd like to see the option tweaked a little bit to be slightly less confusing, but otherwise this is generally OK.

::: toolkit/moz.configure:698
(Diff revision 3)
>  set_config('SERVO_TARGET_DIR', servo_target_dir)
>  
>  # WebRender integration
>  option('--enable-webrender', help='Include WebRender')
>  
> -set_config('MOZ_BUILD_WEBRENDER',
> +@depends('--enable-webrender', milestone, target)

This is mostly good, I just think that the semantics of explicit `--enable-webrender` vs. implicit are a bit confusing and we should tweak this a bit. We have [something similar for ICU](https://dxr.mozilla.org/mozilla-central/rev/05bfa2831c0ba4a26fa72328ffe6a99aba9c356a/build/autoconf/icu.m4#25), where we support:
* `--with-intl-api` - the default
* `--without-intl-api` - to disable it entirely
* `--with-intl-api=build` to build the code but not enable the APIs

I think this option would work better as:
* `--enable-webrender=build` - the default on non-Android nightly
* `--enable-webrender` - when explicitly specified, build and enable
* `--disable-webrender` - don't build webrender

The ICU example above is still in old-configure, but the [`--enable-jemalloc`](https://dxr.mozilla.org/mozilla-central/rev/05bfa2831c0ba4a26fa72328ffe6a99aba9c356a/build/moz.configure/memory.configure#12) option does something very similar. I think your existing code here would need very little change to work this way.

::: toolkit/moz.configure:729
(Diff revision 3)
> +    # in all other cases, don't enable it
> +    return None
> +
> +set_config('MOZ_BUILD_WEBRENDER', build_webrender)
> +set_define('MOZ_BUILD_WEBRENDER', build_webrender)
> +set_config('MOZ_ENABLE_WEBRENDER', enable_webrender)

If you're not actively using this for anything I would just leave it out. We can always easily add it in the future if we need it for something.

::: toolkit/moz.configure:730
(Diff revision 3)
> +    return None
> +
> +set_config('MOZ_BUILD_WEBRENDER', build_webrender)
> +set_define('MOZ_BUILD_WEBRENDER', build_webrender)
> +set_config('MOZ_ENABLE_WEBRENDER', enable_webrender)
> +set_define('MOZ_ENABLE_WEBRENDER', enable_webrender)

I don't think this needs to be a global define, you only need it for the prefs file so you can just set it in DEFINES in the moz.build file there. Global defines make for more rebuilds since they apply to every source file.
Attachment #8846795 - Flags: review-
Comment on attachment 8846795 [details]
Bug 1342450 - Extract a MOZ_ENABLE_WEBRENDER from MOZ_BUILD_WEBRENDER so that we build but disable by default.

https://reviewboard.mozilla.org/r/119806/#review124962

::: toolkit/moz.configure:727
(Diff revision 4)
> +set_config('MOZ_BUILD_WEBRENDER', build_webrender)
> +set_define('MOZ_BUILD_WEBRENDER', build_webrender)
> +set_config('MOZ_ENABLE_WEBRENDER', enable_webrender)
> +set_define('MOZ_ENABLE_WEBRENDER', enable_webrender)

Given the duplicated logic between these functions, I wonder if it'd be better for you to have something like:

```
@depends('--enable-webrender', milestone, target)
def webrender(value, mileston, target):
  build_webrender = False
  enable_webrender = False
  
  if target.os == 'Android':
    # we can't yet build WebRender on Android...
    pass
  elif value.origin == 'default':
    # if unspecified, build by default on Nightly
    build_webrender = milestone.is_nightly
  elif bool(value):
    # if set to true explicitly, turn it on
    build_webrender = True
    enable_webrender = True

  # ensure we've not botched the logic above
  if enable_webrender:
    assert build_webrender

  # in all other cases, the defaults are fine
  return namespace(
    build=build_webrender,
    enable=enable_webrender,
  )
  
set_config('MOZ_BUILD_WEBRENDER', delayed_getattr(webrender, 'build'))
...
set_config('MOZ_ENABLE_WEBRENDER', delayed_getattr(webrender, 'enable'))
...
```

I think this makes it easier for subsequent modifications to keep everything in sync.  WDYT?

Given that this has waited so long, I'll r+ this version, but I'll open a mozreview issue so you can decide if you want to fix it here (along with retesting) or open a followup bug for the cleanup.
Attachment #8846795 - Flags: review?(nfroyd) → review+
I see we review-collided; Ted has some good points that are at least worth discussing before this lands.
All of those suggestions make sense, here's a try push with the updates:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b61bc0bea77cb80fe04434dfdd581fbd0072db1

I'll re-request review if that looks good.
Comment on attachment 8846795 [details]
Bug 1342450 - Extract a MOZ_ENABLE_WEBRENDER from MOZ_BUILD_WEBRENDER so that we build but disable by default.

https://reviewboard.mozilla.org/r/119806/#review124924

> If you're not actively using this for anything I would just leave it out. We can always easily add it in the future if we need it for something.

Since I moved the set_define('MOZ_ENABLE_WEBRENDER', enable_webrender) out into the modules/libpref/moz.build file as a DEFINES update, I needed to keep the config value here. I think I addressed all the other issues.
Comment on attachment 8846796 [details]
Bug 1342450 - Keep webrender disabled by default on OS X buildbot builders.

https://reviewboard.mozilla.org/r/119808/#review125090
Attachment #8846796 - Flags: review+
Comment on attachment 8846795 [details]
Bug 1342450 - Extract a MOZ_ENABLE_WEBRENDER from MOZ_BUILD_WEBRENDER so that we build but disable by default.

https://reviewboard.mozilla.org/r/119806/#review125092

Looks good, thanks for making that change!
Attachment #8846795 - Flags: review+
Comment on attachment 8846796 [details]
Bug 1342450 - Keep webrender disabled by default on OS X buildbot builders.

Thanks!
Attachment #8846796 - Flags: review?(nfroyd)
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e6a7c83e474
Rename MOZ_ENABLE_WEBRENDER to MOZ_BUILD_WEBRENDER. r=rhunt
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e4575ad9ce7
Extract a MOZ_ENABLE_WEBRENDER from MOZ_BUILD_WEBRENDER so that we build but disable by default. r=rhunt,froydnj,ted
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d9e9c6020bd
Keep webrender disabled by default on OS X buildbot builders. r=ted
https://hg.mozilla.org/mozilla-central/rev/2e6a7c83e474
https://hg.mozilla.org/mozilla-central/rev/7e4575ad9ce7
https://hg.mozilla.org/mozilla-central/rev/9d9e9c6020bd
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1350001
Depends on: 1352092
No longer depends on: 1352092
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: