Replace MOZ_MUST_USE with [[nodiscard]] in toolkit/components/antitracking
Categories
(Core :: Privacy: Anti-Tracking, task, P3)
Tracking
()
People
(Reporter: cpeterson, Assigned: cpeterson)
References
Details
(Keywords: perf-alert)
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
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.
Assignee | ||
Comment 1•4 years ago
|
||
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.
Pushed by cpeterson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0b67ba9a966c Replace MOZ_MUST_USE with [[nodiscard]] in toolkit/components/antitracking. r=dimi
Comment 3•4 years ago
|
||
bugherder |
Comment 4•4 years ago
|
||
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
Updated•4 years ago
|
Updated•4 years ago
|
Assignee | ||
Comment 5•4 years ago
|
||
(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:
Looks like a slight regression may have started on December 2 before my change, perhaps related to perf alert bug 1680450:
With the following pushlog, which includes gfx bug 1679577, which disabled D3D11 double buffering and Advanced Layers on Windows. Perhaps that's related?
Looks like only Windows has gone bimodal. macOS and Linux appear unchanged:
Since this is a perf improvement, I don't think we need to spend much more time investigating. :)
Description
•