Closed Bug 1193437 Opened 5 years ago Closed 3 months ago

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

Categories

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

38 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla80
Tracking Status
firefox-esr78 --- fixed
firefox79 --- fixed
firefox80 --- fixed
Blocking Flags:
backlog tech-debt

People

(Reporter: ekr, Assigned: bwc)

References

Details

Attachments

(1 file)

NrIceCtx::Create has accumulated a pile of boolean switches. Convert these to some sort of flag.
backlog: --- → tech-debt
Rank: 45
Priority: -- → P4
Mass change P4->P5 to align with new Mozilla triage process.
Priority: P4 → P5

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

Assignee: nobody → docfaraday
Priority: P5 → P2

Try looks ok.

Pushed by bcampen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e1841978b15a
Use config structs in NrIceCtx, and centralize pref lookups some. r=mjf

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?

Flags: needinfo?(docfaraday) → needinfo?(mh+mozilla)

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?

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?

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.

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

Pushed by bcampen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b016fd854492
Use config structs in NrIceCtx, and centralize pref lookups some. r=mjf
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla80

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

Flags: needinfo?(docfaraday)

No, those come from bug 1637544.

Flags: needinfo?(mh+mozilla)
Flags: needinfo?(docfaraday)
Duplicate of this bug: 1649222

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
Attachment #9161903 - Flags: approval-mozilla-beta?

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
Attachment #9161903 - Flags: approval-mozilla-esr78?

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.

Attachment #9161903 - Flags: approval-mozilla-esr78?
Attachment #9161903 - Flags: approval-mozilla-esr78+
Attachment #9161903 - Flags: approval-mozilla-beta?
Attachment #9161903 - Flags: approval-mozilla-beta+

Ah, yes, rot from bug 1648010.

Flags: needinfo?(docfaraday)

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

Attachment #9161903 - Attachment description: Bug 1193437: Use config structs in NrIceCtx, and centralize pref lookups some. r?mjf → Bug 1193437: (79 backport) Use config structs in NrIceCtx, and centralize pref lookups some. r=mjf
You need to log in before you can comment on or make changes to this bug.