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)
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: kats, Assigned: kats)
References
(Depends on 1 open bug)
Details
(Whiteboard: [gfx-noted])
Attachments
(3 files)
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
Assignee | ||
Comment 1•8 years ago
|
||
(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/
Assignee | ||
Comment 2•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a09ffb2e287e6270ddec04e41e6c5c0cab757c4
Assignee | ||
Comment 3•8 years ago
|
||
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?
Assignee | ||
Comment 4•8 years ago
|
||
(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.
Assignee | ||
Comment 5•8 years ago
|
||
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
Assignee | ||
Comment 6•8 years ago
|
||
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.
Assignee | ||
Comment 7•8 years ago
|
||
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.
Assignee | ||
Comment 8•8 years ago
|
||
Latest try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ff236bb4a2cb6d2696edd697e93056e7b6fb9c4
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•8 years ago
|
||
Shoot, I just realized this will break local Android builds. Updated part 2/3 coming.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 16•8 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 17•8 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 21•8 years ago
|
||
mozreview-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/#review123042
Attachment #8846795 -
Flags: review?(rhunt) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 25•8 years ago
|
||
^ Rebased to master, and s/ted/froydnj/ since ted has been pretty busy, and Nathan graciously accepted to do the reviews instead.
Comment 26•8 years ago
|
||
mozreview-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/#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 27•8 years ago
|
||
mozreview-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+
![]() |
||
Comment 28•8 years ago
|
||
I see we review-collided; Ted has some good points that are at least worth discussing before this lands.
Assignee | ||
Comment 29•8 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 33•8 years ago
|
||
mozreview-review-reply |
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 34•8 years ago
|
||
mozreview-review |
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 35•8 years ago
|
||
mozreview-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+
Assignee | ||
Comment 36•8 years ago
|
||
Comment on attachment 8846796 [details] Bug 1342450 - Keep webrender disabled by default on OS X buildbot builders. Thanks!
Attachment #8846796 -
Flags: review?(nfroyd)
Comment 37•8 years ago
|
||
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
Comment 38•8 years ago
|
||
bugherder |
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
Updated•7 years ago
|
status-firefox54:
affected → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•