1193437 - NrIceCtx::Create() has too many bools; convert to flags

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P2)

Description

[Eric Rescorla (:ekr)]

NrIceCtx::Create has accumulated a pile of boolean switches. Convert these to some sort of flag.

Comment 1

[Bulk Bug Changes for mreavy's org]

Mass change P4->P5 to align with new Mozilla triage process.

Comment 2

[Byron Campen [:bwc]]

Let's go ahead and do this. We're grabbing config values all over the place, and it is getting hard to read.

Comment 3

[Byron Campen [:bwc]]

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

Gtest: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2eaefff1f201ed7c4df9868d8a2e22551e2e1334

wpt (needs separate push due to bug 1649855): https://treeherder.mozilla.org/#/jobs?repo=try&revision=e535a53aa5643a337465e23b850adc0a4c3b2c05

Comment 4

[Byron Campen [:bwc]]

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

Comment 5

[Byron Campen [:bwc]]

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

Comment 6

[Byron Campen [:bwc]]

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

Comment 7

[Byron Campen [:bwc]]

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

Comment 8

[Byron Campen [:bwc]]

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

Comment 9

[Byron Campen [:bwc]]

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

Comment 10

[Byron Campen [:bwc]]

Try looks ok.

Comment 11

[Byron Campen [:bwc]]

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

Comment 12

[Byron Campen [:bwc]]

Attachment: Bug 1193437: (79 backport) Use config structs in NrIceCtx, and centralize pref lookups some. r=mjf

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e1841978b15a
Use config structs in NrIceCtx, and centralize pref lookups some. r=mjf

Comment 14

[Narcis Beleuzu [:NarcisB]]

Backed out for bustages on ice_unittest.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/99f760e933bfbcace9877fe76c81781caf3e9266
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=308928864&repo=autoland&lineNumber=27383

Comment 15

[Byron Campen [:bwc]]

Huh. The only failure is in a base-toolchains build, which seems to be intended to quickly catch cases where one of our "real" toolchains is insufficiently modern. However, all of the other compilations are happy; do we need to bump one of the toolchains in the base-toolchains job?

Comment 16

[Byron Campen [:bwc]]

It does not look like gcc 7 is supported anymore (by the GNU project). Is that where we're tripping up? Does gcc 8 support this syntax?

Comment 17

[Byron Campen [:bwc]]

So it looks like the one holdout for gcc 7 is ccov builds (which weren't run on the autoland push for this bug). Those probably could use gcc 8?

Comment 18

[Byron Campen [:bwc]]

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

Comment 19

[Byron Campen [:bwc]]

It looks like the base-toolchain builds are unhappy with some code in other places if they use gcc 8, although gcc 8 seems to be ok with the syntax I'm using here. I guess I'll have to fall back to the clunkier syntax.

Comment 20

[Byron Campen [:bwc]]

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

Comment 21

[Byron Campen [:bwc]]

Waiting for some retriggers, but try looks good so far.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b016fd854492
Use config structs in NrIceCtx, and centralize pref lookups some. r=mjf

Comment 23

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/b016fd854492

Comment 24

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Byron, does this improvement makes sense?
== Change summary for alert #26495 (as of Mon, 13 Jul 2020 10:30:01 GMT) ==

Improvements:

55% build times linux64-shippable opt instrumented taskcluster-m5.4xlarge 2,225.72 -> 1,002.70
52% build times osx-shippable opt instrumented taskcluster-m5.4xlarge 2,401.25 -> 1,149.88
49% build times linux64-shippable opt instrumented taskcluster-c5d.4xlarge 2,137.52 -> 1,098.07
47% build times osx-shippable opt instrumented taskcluster-c5.4xlarge 2,312.68 -> 1,225.96
45% build times android-4-0-armv7-api16 pgo instrumented taskcluster-c5d.4xlarge 1,767.90 -> 969.43
42% build times osx-shippable opt instrumented taskcluster-c5d.4xlarge 2,318.09 -> 1,334.99
41% build times android-4-0-armv7-api16 pgo instrumented taskcluster-c5.4xlarge 1,807.32 -> 1,070.22

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=26495

Comment 25

[Mike Hommey [:glandium]]

No, those come from bug 1637544.

Comment 27

[Byron Campen [:bwc]]

Comment on attachment 9161903 [details]
Bug 1193437: (79 backport) Use config structs in NrIceCtx, and centralize pref lookups some. r=mjf

Beta/Release Uplift Approval Request

• User impact if declined:
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):
• String changes made/needed: None

Comment 28

[Byron Campen [:bwc]]

Comment on attachment 9161903 [details]
Bug 1193437: (79 backport) Use config structs in NrIceCtx, and centralize pref lookups some. r=mjf

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined:
• Fix Landed on Version: 80
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):
• String or UUID changes made by this patch: None

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9161903 [details]
Bug 1193437: (79 backport) Use config structs in NrIceCtx, and centralize pref lookups some. r=mjf

Approved for 79.0b9 and 78.1esr.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d1bb79e94137

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Backed out for Beta build bustage:
https://hg.mozilla.org/releases/mozilla-beta/rev/8b8c4c23c0bcf047000ec959931f5df2b57a08ea

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310034497&repo=mozilla-beta&lineNumber=16606

Comment 32

[Byron Campen [:bwc]]

Ah, yes, rot from bug 1648010.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Trivial to rebase around at least. I've got a green Try push and will re-land :)

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f36438f7dd67

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/58e9540c9888