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

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Attachment: Add MOZ_DISABLE_CONTEXT_SHARING_GLX for Cipc/Ripc

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.

Comment 1

[[e-mail] (:bhearsum)]

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.

Comment 2

[Nick Cameron [:nrc]]

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?

Comment 3

[[e-mail] (: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...

Comment 4

[[e-mail] (: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?

Comment 5

[Matt Woodrow (:mattwoodrow)]

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.

Comment 6

[[e-mail] (:bhearsum)]

(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.

Comment 7

[[e-mail] (:bhearsum)]

Attachment: add new flags

Comment 8

[[e-mail] (:bhearsum)]

Attachment: add new flags

I guess I should post a real patch....

Comment 9

[[e-mail] (:bhearsum)]

Comment on attachment 733963 [details] [diff] [review]
add new flags

Landed on default - should get put into production on Monday.

Comment 10

[Kim Moir [:kmoir] ET]

in production

Comment 11

[[e-mail] (:bhearsum)]

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.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

So, this torched Cipc/Ripc on the b2g18 branches...

Comment 13

[[e-mail] (:bhearsum)]

Comment on attachment 733963 [details] [diff] [review]
add new flags

Ugh, I was afraid of that. I'm backing out.

Comment 14

[Kim Moir [:kmoir] ET]

reverted https://hg.mozilla.org/build/buildbotcustom/rev/80125240d77c

Comment 15

[[e-mail] (:bhearsum)]

Matt, would it hurt to keep the MOZ_LAYERS_FORCE_SHMEM_SURFACES flag? Removing it seems to have busted some branches.

Comment 16

[Matt Woodrow (:mattwoodrow)]

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 17

[[e-mail] (:bhearsum)]

(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?

Comment 18

[Matt Woodrow (:mattwoodrow)]

Unfortunately that comment was wrong :(

My intent was for both to be new, but that wasn't what actually landed.

Comment 19

[Nick Cameron [:nrc]]

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?

Comment 20

[Nick Cameron [:nrc]]

Attachment: add flags and pref

Use a fresh flag and pref

Comment 21

[[e-mail] (:bhearsum)]

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.

Comment 22

[[e-mail] (:bhearsum)]

Aki, we recently moved unit tests to mozharness. Do we have any known way in those scripts for train riding / branch specific settings?

Comment 23

[Aki Sasaki (not active)]

(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.

Comment 24

[Nick Cameron [:nrc]]

(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.

Comment 25

[[e-mail] (: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?

Comment 26

[Nick Cameron [:nrc]]

(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.

Comment 27

[Nick Cameron [:nrc]]

Attachment: add flags and pref

now without removing the surfaces flag

Comment 28

[[e-mail] (:bhearsum)]

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.

Comment 29

[Nick Cameron [:nrc]]

Landed on hg.mozilla.org/build/buildbotcustom/

Comment 30

[Chris AtLee [:catlee]]

in production

Comment 31

[Nick Thomas [:nthomas] (UTC+12)]

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.

Comment 32

[Matt Woodrow (:mattwoodrow)]

Attachment: Add prefs to mozharness too

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?

Comment 33

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 735553 [details] [diff] [review]
Add prefs to mozharness too

Forwarding to aki, who wrote this code.

Comment 34

[Aki Sasaki (not active)]

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.

Comment 35

[Matt Woodrow (:mattwoodrow)]

Attachment: Add env vars to mozharness

Hope this is what you meant.

Obviously untested, not sure how I'd go about doing that.

Comment 36

[Matt Woodrow (:mattwoodrow)]

Attachment: Add env vars to mozharness v2

Added crashtest-ipc changes too, and fixed the syntax error.

Comment 37

[Aki Sasaki (not active)]

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.

Comment 38

[Nick Thomas [:nthomas] (UTC+12)]

Landed attachment 735553 [details] [diff] [review] and 735575 as 
 http://hg.mozilla.org/build/mozharness/rev/e2d502f58764

Comment 39

[Matt Woodrow (:mattwoodrow)]

Landed a follow-up because I typo'd a variable name: http://hg.mozilla.org/build/mozharness/rev/2a81f599721d

Comment 40

[Aki Sasaki (not active)]

Merged to production.

Comment 41

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 735373 [details] [diff] [review]
add flags and pref

Backed this out:
 http://hg.mozilla.org/build/buildbotcustom/rev/21ea12b25bdb

Comment 42

[[e-mail] (:bhearsum)]

Aki and Nick - thanks for jumping in here. How're things looking now, Matt?

Comment 43

[Matt Woodrow (:mattwoodrow)]

Things are great, we've landed on m-c and tests are passing.

Thanks for all the help getting this fixed in a hurry.

Comment 44

[Rail Aliiev [:rail]]

in production