Closed Bug 854758 Opened 12 years ago Closed 12 years ago

Set the MOZ_DISABLE_CONTEXT_SHARING_GLX env var for Cipc/Ripc on mozilla-central and related branches

Categories

(Release Engineering :: General, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mattwoodrow, Assigned: nrc)

References

Details

Attachments

(3 files, 5 obsolete files)

With the graphics refactoring, we appear to be hitting a driver bug on Cipc/Ripc. This env var disables context sharing which is the only workaround we know of. The existing env var 'MOZ_LAYERS_FORCE_SHMEM_SURFACES' doesn't actually appear to be used for anything any more.
Attachment #729368 - Flags: review?(lsblakk)
Comment on attachment 729368 [details] [diff] [review] Add MOZ_DISABLE_CONTEXT_SHARING_GLX for Cipc/Ripc We should try to make this ride the trains...I'll see if I can figure something out.
Attachment #729368 - Flags: review?(lsblakk)
Turns out we also need to add the MOZ_USE_OMTC flag for these tests too. Could you add that in as well? Do you want a new patch or will you be doing this by hand anyway?
Flags: needinfo?(bhearsum)
(In reply to Nick Cameron [:nrc] from comment #2) > Turns out we also need to add the MOZ_USE_OMTC flag for these tests too. > Could you add that in as well? Do you want a new patch or will you be doing > this by hand anyway? There's no doing it by hand, but don't bother with a new patch. I still haven't figured out how to make these changes ride the trains...
Flags: needinfo?(bhearsum)
I think I have a way to do this provided it's safe to set this pref for all unit tests. Matt, any idea if that would have any negative impact?
Flags: needinfo?(matt.woodrow)
I *think* that would be ok, but it would probably cause issues if we try enable OpenGL layers on the test machines. I guess we're unlikely to do that on the ec2 boxes though, since their software OpenGL is the reason we need to do this. Is there any reason we need to make this ride the trains? Both env vars that we're adding are new ones, so should have no effect if applied to other trees. Unless the existing var that's being removed still exists in older version of the code.
Flags: needinfo?(matt.woodrow)
(In reply to Matt Woodrow (:mattwoodrow) from comment #5) > I *think* that would be ok, but it would probably cause issues if we try > enable OpenGL layers on the test machines. > > I guess we're unlikely to do that on the ec2 boxes though, since their > software OpenGL is the reason we need to do this. > > Is there any reason we need to make this ride the trains? Both env vars that > we're adding are new ones, so should have no effect if applied to other > trees. I was more concerned about MOZ_LAYERS_FORCE_SHMEM_SURFACES, to be honest. However, given that it only affects debug builds (https://hg.mozilla.org/releases/mozilla-esr17/file/default/gfx/layers/ipc/ShadowLayers.cpp#l452) I think it's probably okay to change...we should just watch out for new test failures on aurora/beta/release/esr17 when landing. I'll whip up a new patch.
Attached patch add new flags (obsolete) (deleted) — Splinter Review
Assignee: nobody → bhearsum
Attachment #729368 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #733845 - Flags: review?(rail)
Attached patch add new flags (obsolete) (deleted) — Splinter Review
I guess I should post a real patch....
Attachment #733845 - Attachment is obsolete: true
Attachment #733845 - Flags: review?(rail)
Attachment #733963 - Flags: review?(rail)
Attachment #733963 - Flags: review?(rail) → review+
Comment on attachment 733963 [details] [diff] [review] add new flags Landed on default - should get put into production on Monday.
Attachment #733963 - Flags: checked-in+
in production
Matt, anything you want to do to verify that this is working as expected? Job started after Kim's comment should have the new flags set.
So, this torched Cipc/Ripc on the b2g18 branches...
Comment on attachment 733963 [details] [diff] [review] add new flags Ugh, I was afraid of that. I'm backing out.
Attachment #733963 - Flags: checked-in+ → checked-in-
Matt, would it hurt to keep the MOZ_LAYERS_FORCE_SHMEM_SURFACES flag? Removing it seems to have busted some branches.
I believe it's actually the MOZ_USE_OMTC env var that we are adding. We should change this to a new name, and update the graphics branch so that it reads the new var.
(In reply to Matt Woodrow (:mattwoodrow) from comment #16) > I believe it's actually the MOZ_USE_OMTC env var that we are adding. > > We should change this to a new name, and update the graphics branch so that > it reads the new var. comment #5 says both env vars are new. Doesn't that mean MOZ_USE_OMTC should be unused on b2g18 (which is Gecko 18-based), or did I misunderstand something?
Unfortunately that comment was wrong :( My intent was for both to be new, but that wasn't what actually landed.
Looks like we also need to set the layers.offmainthreadcomposition.enabled pref to true for these runs too. Can someone tell me how to do this please?
Attached patch add flags and pref (obsolete) (deleted) — Splinter Review
Use a fresh flag and pref
Attachment #733963 - Attachment is obsolete: true
Attachment #734877 - Flags: review?(rail)
Comment on attachment 734877 [details] [diff] [review] add flags and pref (In reply to Nick Cameron [:nrc] from comment #19) > Looks like we also need to set the layers.offmainthreadcomposition.enabled > pref to true for these runs too. Can someone tell me how to do this please? Unfortunately, it's not this this simple anymore. Because the OMTC flag/pref affects older branches we need to make sure that this is only set on mozilla-central (and related) branches, and that it rides the trains forward.
Attachment #734877 - Flags: review?(rail) → review-
Aki, we recently moved unit tests to mozharness. Do we have any known way in those scripts for train riding / branch specific settings?
Flags: needinfo?(aki)
Assignee: bhearsum → nobody
Component: Release Engineering → Release Engineering: Automation (General)
QA Contact: catlee
Summary: [buildbotcustom] Set the MOZ_DISABLE_CONTEXT_SHARING_GLX env var for Cipc/Ripc → Set the MOZ_DISABLE_CONTEXT_SHARING_GLX env var for Cipc/Ripc on mozilla-central and related branches
(In reply to Ben Hearsum [:bhearsum] from comment #22) > Aki, we recently moved unit tests to mozharness. Do we have any known way in > those scripts for train riding / branch specific settings? Lots of options, but nothing implemented in a really easy way yet. * different command line options or buildprops per branch. This will require buildbot-configs changes and a reconfig every merge day. * looking at something in-tree or in-test-zip for clues in behavior. * multiple config files. However, currently these are required to all exist, so if you did script.py -c real_config.py -c BRANCH_config.py then BRANCH_config.py would have to exist for every single branch we run tests for. Maybe hackable like we did generic mozconfigs. This would require a mozharness config change every merge day. Some options that will require more work: * multiple config files. If we made an optional additional config file, e.g. -c required_config -C optional_config, then optional_config could be path/to/OPTIONAL_CONFIG.py (possibly an in-tree/in-test-zip file) * optional config file via url, pointing to hg.m.o/FULL_BRANCH_PATH/raw/FILE * I have mozharness talos look in-tree for a talos.json that has a lot of config info; we could do something similar for unittests. * Possibly a way for unittests to determine what version Firefox is, and specifying "turn this config option on for fx version >= X or w/e) Possibly other solutions; these are the ones that come to mind.
Flags: needinfo?(aki)
(In reply to Ben Hearsum [:bhearsum] from comment #21) > Comment on attachment 734877 [details] [diff] [review] > add flags and pref > > (In reply to Nick Cameron [:nrc] from comment #19) > > Looks like we also need to set the layers.offmainthreadcomposition.enabled > > pref to true for these runs too. Can someone tell me how to do this please? > > Unfortunately, it's not this this simple anymore. Because the OMTC flag/pref > affects older branches we need to make sure that this is only set on > mozilla-central (and related) branches, and that it rides the trains forward. I have used a completely new env var and pref, neither of which are used in existing code, and changed the code on the graphics branch to check for either the new or old pref. So existing code should no longer be affected.
Attachment #734877 - Flags: review- → review?(bhearsum)
(In reply to Nick Cameron [:nrc] from comment #24) > (In reply to Ben Hearsum [:bhearsum] from comment #21) > > Comment on attachment 734877 [details] [diff] [review] > > add flags and pref > > > > (In reply to Nick Cameron [:nrc] from comment #19) > > > Looks like we also need to set the layers.offmainthreadcomposition.enabled > > > pref to true for these runs too. Can someone tell me how to do this please? > > > > Unfortunately, it's not this this simple anymore. Because the OMTC flag/pref > > affects older branches we need to make sure that this is only set on > > mozilla-central (and related) branches, and that it rides the trains forward. > > I have used a completely new env var and pref, neither of which are used in > existing code, and changed the code on the graphics branch to check for > either the new or old pref. So existing code should no longer be affected. Ah, sorry. I only searched for "offmainthreadcomposition" previously, and confused some of the results with the actual pref you're setting. However, when we landed the previous patch it busted b2g18, so _something_ in there is affecting it. I see that MOZ_LAYERS_FORCE_SHMEM_SURFACES exists on that branch, so maybe the removal of it caused that problem?
(In reply to Ben Hearsum [:bhearsum] from comment #25) > (In reply to Nick Cameron [:nrc] from comment #24) > > (In reply to Ben Hearsum [:bhearsum] from comment #21) > > > Comment on attachment 734877 [details] [diff] [review] > > > add flags and pref > > > > > > (In reply to Nick Cameron [:nrc] from comment #19) > > > > Looks like we also need to set the layers.offmainthreadcomposition.enabled > > > > pref to true for these runs too. Can someone tell me how to do this please? > > > > > > Unfortunately, it's not this this simple anymore. Because the OMTC flag/pref > > > affects older branches we need to make sure that this is only set on > > > mozilla-central (and related) branches, and that it rides the trains forward. > > > > I have used a completely new env var and pref, neither of which are used in > > existing code, and changed the code on the graphics branch to check for > > either the new or old pref. So existing code should no longer be affected. > > Ah, sorry. I only searched for "offmainthreadcomposition" previously, and > confused some of the results with the actual pref you're setting. > > However, when we landed the previous patch it busted b2g18, so _something_ > in there is affecting it. I see that MOZ_LAYERS_FORCE_SHMEM_SURFACES exists > on that branch, so maybe the removal of it caused that problem? We think what affected it was the env var - MOZ_USE_OMTC, but in the new patch I use MOZ_OMTC_ENABLED, which has not been used before. We could leave MOZ_LAYERS_FORCE_SHMEM_SURFACES to be on the safe side.
Attached patch add flags and pref (deleted) — Splinter Review
now without removing the surfaces flag
Attachment #734877 - Attachment is obsolete: true
Attachment #734877 - Flags: review?(bhearsum)
Attachment #735373 - Flags: review?(bhearsum)
Assignee: nobody → ncameron
Comment on attachment 735373 [details] [diff] [review] add flags and pref Review of attachment 735373 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Nick Cameron [:nrc] from comment #26) > (In reply to Ben Hearsum [:bhearsum] from comment #25) > > (In reply to Nick Cameron [:nrc] from comment #24) > > > (In reply to Ben Hearsum [:bhearsum] from comment #21) > > > > Comment on attachment 734877 [details] [diff] [review] > > > > add flags and pref > > > > > > > > (In reply to Nick Cameron [:nrc] from comment #19) > > > > > Looks like we also need to set the layers.offmainthreadcomposition.enabled > > > > > pref to true for these runs too. Can someone tell me how to do this please? > > > > > > > > Unfortunately, it's not this this simple anymore. Because the OMTC flag/pref > > > > affects older branches we need to make sure that this is only set on > > > > mozilla-central (and related) branches, and that it rides the trains forward. > > > > > > I have used a completely new env var and pref, neither of which are used in > > > existing code, and changed the code on the graphics branch to check for > > > either the new or old pref. So existing code should no longer be affected. > > > > Ah, sorry. I only searched for "offmainthreadcomposition" previously, and > > confused some of the results with the actual pref you're setting. > > > > However, when we landed the previous patch it busted b2g18, so _something_ > > in there is affecting it. I see that MOZ_LAYERS_FORCE_SHMEM_SURFACES exists > > on that branch, so maybe the removal of it caused that problem? > > We think what affected it was the env var - MOZ_USE_OMTC, but in the new > patch I use MOZ_OMTC_ENABLED, which has not been used before. We could leave > MOZ_LAYERS_FORCE_SHMEM_SURFACES to be on the safe side. Ah, I see. This seems OK now. Feel free to land it on the default branch at your convenience. It'll get put into production with the next reconfig.
Attachment #735373 - Flags: review?(bhearsum) → review+
Landed on hg.mozilla.org/build/buildbotcustom/
in production
I think this is failing to have any effect because m-c & co switched to running these tests through mozharness, rather than through the buildbot harnesses. The patch is making changes on branches like mozilla-b2g18*, mozilla-esr17 and mozilla-release because they still do things the old way.
Attached patch Add prefs to mozharness too (deleted) — Splinter Review
It appears (from looking at log output) that we are only getting pref values from here, and not from buildbotcustom. This is a patch to add the pref to mozharness, but doesn't add the env vars that we need. Any idea how I can do that?
Attachment #735553 - Flags: review?(nthomas)
Comment on attachment 735553 [details] [diff] [review] Add prefs to mozharness too Forwarding to aki, who wrote this code.
Attachment #735553 - Flags: review?(nthomas) → review?(aki)
Comment on attachment 735553 [details] [diff] [review] Add prefs to mozharness too Jordan wrote it :) This will turn this flag on for linux cipc + ripc on m-c (and all m-c level repos, including try and inbound), and m-a. If that's the desired effect, r=me. This should land in default, and we can merge into the production mozharness branch when tests pass.
Attachment #735553 - Flags: review?(aki) → review+
Attached patch Add env vars to mozharness (obsolete) (deleted) — Splinter Review
Hope this is what you meant. Obviously untested, not sure how I'd go about doing that.
Attachment #735564 - Flags: review?(aki)
Attached patch Add env vars to mozharness v2 (deleted) — Splinter Review
Added crashtest-ipc changes too, and fixed the syntax error.
Attachment #735564 - Attachment is obsolete: true
Attachment #735564 - Flags: review?(aki)
Attachment #735575 - Flags: review?(aki)
Comment on attachment 735575 [details] [diff] [review] Add env vars to mozharness v2 If you land on default, Cedar unittests will be affected and nothing else. You or Nick or a sheriff can trigger tests to verify (or merge m-c in or w/e). Merging to the production branch will roll out everywhere.
Attachment #735575 - Flags: review?(aki) → review+
Landed a follow-up because I typo'd a variable name: http://hg.mozilla.org/build/mozharness/rev/2a81f599721d
Merged to production.
Aki and Nick - thanks for jumping in here. How're things looking now, Matt?
Things are great, we've landed on m-c and tests are passing. Thanks for all the help getting this fixed in a hurry.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
in production
Product: mozilla.org → Release Engineering
Component: General Automation → General
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: