Closed
Bug 1431223
Opened 6 years ago
Closed 6 years ago
Don't use do-while(false) for non-debug assert in self-hosted code
Categories
(Core :: JavaScript Engine, defect, P1)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: anba, Assigned: anba)
Details
Attachments
(1 file)
1.20 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
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()); ---
Comment 1•6 years ago
|
||
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...
Assignee | ||
Comment 2•6 years ago
|
||
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 3•6 years ago
|
||
Comment on attachment 8943613 [details] [diff] [review] bug1431223.patch Review of attachment 8943613 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #8943613 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 4•6 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba81caf6b8f14ed0315f504a1163f58acda8e4b6
Keywords: checkin-needed
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
Comment 6•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/aadd06ac89ab
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in
before you can comment on or make changes to this bug.
Description
•