Closed Bug 1431223 Opened 2 years ago Closed 2 years ago

Don't use do-while(false) for non-debug assert in self-hosted code

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: anba, Assigned: anba)

Details

Attachments

(1 file)

This µ-benchmark improves from 480ms to 415ms for me when I replace |do {} while (false)| with |;| (an empty-statement) in [1]. If this improvement is not just some strange effect on my (virtual) machine, it seems worthwhile to replace the do-while statement with an empty statement.

[1] https://searchfox.org/mozilla-central/rev/1f9b4f0f0653b129b4db1b9fc5e9068054afaac0/js/src/builtin/Utilities.js#43

---
function f() {
    var a = Array(100).fill(0);
    var q = 0;
    var t = dateNow();
    for (var i = 0; i < 1000000; ++i) {
        for (var k of a.keys()) {
            q += k;
        }
    }
    return [dateNow() - t, q];
}
for (var i = 0; i < 10; ++i) print(f());
---
Ouch, great catch! In theory we could optimize away |do {} while (false);| in the bytecode emitter, but in practice that probably messes with things like debugging...
Attached patch bug1431223.patchSplinter Review
The do-while loop was initially added because of this review comment in bug 1374239 comment #2. Using an empty statement instead of a do-while statement should still cover the issue noted by :shu without leading to the performance issue noted in comment #0.
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8943613 - Flags: review?(jdemooij)
Comment on attachment 8943613 [details] [diff] [review]
bug1431223.patch

Review of attachment 8943613 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #8943613 - Flags: review?(jdemooij) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aadd06ac89ab
Use an empty-statement for assert/dbg in non-debug builds. r=jandem
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/aadd06ac89ab
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.