1679361 - Replace MOZ_MUST_USE with [[nodiscard]] in toolkit/components/antitracking

Status: RESOLVED FIXED

(Core :: Privacy: Anti-Tracking, task, P3)

Description

[Chris Peterson [:cpeterson]]

The MOZ_MUST_USE macro is defined as clang's and gcc's nonstandard __attribute__((warn_unused_result)). Now that we compile as C++17 by default (bug 1560664), we can replace MOZ_MUST_USE with C++17's standard [[nodiscard]] attribute.

The [[nodiscard]] attribute must precede a function declaration's declaration specifiers (like static, extern, inline, or virtual). The __attribute__((warn_unused_result)) attribute does not have this order restriction.

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: Bug 1679361 - Replace MOZ_MUST_USE with [[nodiscard]] in toolkit/components/antitracking. r?dimi

The MOZ_MUST_USE macro is defined as clang's and gcc's nonstandard __attribute__((warn_unused_result)). Now that we compile as C++17 by default (bug 1560664), we can replace MOZ_MUST_USE with C++17's standard [[nodiscard]] attribute.

The [[nodiscard]] attribute must precede a function declaration's declaration specifiers (like static, extern, inline, or virtual). The __attribute__((warn_unused_result)) attribute does not have this order restriction.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0b67ba9a966c
Replace MOZ_MUST_USE with [[nodiscard]] in toolkit/components/antitracking. r=dimi

Comment 3

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/0b67ba9a966c

Comment 4

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

Hi Chris. The below alert looks like an improvement but the test is getting actually bimodal. What can you say about that?
== Change summary for alert #28328 (as of Tue, 05 Jan 2021 10:57:30 GMT) ==

Improvements:

Ratio	Suite	Test	Platform	Options	Absolute values (old vs new)
11%	tresize		windows10-64-shippable	e10s stylo	8.83 -> 7.86
1%	tresize		windows10-64-shippable	e10s stylo	8.76 -> 8.70

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=28328

Comment 5

[Chris Peterson [:cpeterson]]

(In reply to Alexandru Ionescu (needinfo me) [:alexandrui] from comment #4)

Hi Chris. The below alert looks like an improvement but the test is getting actually bimodal. What can you say about that?

I'm very surprised. My change should have had no impact on the Firefox binary. It just changed some compile-time diagnostics. Perhaps clang is generating better code for a [[nodiscard]] function than for an __attribute__((warn_unused_result)) function? But it really should make no difference and my change only affected one function and it's not on a hot code path.

Looks like the tresize test went bimodal around November 30 for Software WebRender ("tresize opt e10s stylo webrender-sw") before my change:

https://treeherder.mozilla.org/perfherder/graphs?highlightAlerts=1&highlightChangelogData=1&series=autoland,1921208,1,1&series=autoland,2812894,1,1&series=autoland,2813815,1,1&timerange=5184000&zoom=1606022055640,1607484618495,6.458514541680481,11.558066926042592

Looks like a slight regression may have started on December 2 before my change, perhaps related to perf alert bug 1680450:

https://treeherder.mozilla.org/perfherder/graphs?highlightAlerts=1&highlightChangelogData=1&series=autoland,1921208,1,1&timerange=5184000&zoom=1606214989463,1607478394823,6.773554694607009,9.521065383156508

With the following pushlog, which includes gfx bug 1679577, which disabled D3D11 double buffering and Advanced Layers on Windows. Perhaps that's related?

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=124e488aae3abf81f24593043ed3073dcc1645a7&tochange=a4cd97ff70a9db2550000efc69a4358c4db062b3

Looks like only Windows has gone bimodal. macOS and Linux appear unchanged:

https://treeherder.mozilla.org/perfherder/graphs?highlightAlerts=1&highlightChangelogData=1&series=autoland,1921208,1,1&series=autoland,1925670,1,1&series=autoland,2847328,1,1&timerange=5184000

Since this is a perf improvement, I don't think we need to spend much more time investigating. :)