Parse byte code only once in DecompileExpressionFromStack

RESOLVED FIXED in Firefox 63

Status

()

enhancement
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: anba, Assigned: anba)

Tracking

(Blocks 1 bug)

Trunk
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(4 attachments)

No description provided.
Marks most BytecodeParser as |const| in preparation for part 2. And changes the two existing functions accepting |BytecodeParser*| to use |const BytecodeParser*| instead.
Attachment #8999592 - Flags: review?(tcampbell)
Change ExpressionDecompiler to use an existing BytecodeParser instance instead of creating its own BytecodeParser. This avoids to parse the byte code twice when called from DecompileExpressionFromStack.

Part 3 will ensure DecompileExpressionFromStack won't create a BytecodeParser when JSDVG_IGNORE_STACK is used or we have an Ion-frame [1].

[1] https://searchfox.org/mozilla-central/rev/ca869724246f4230b272ed1c8b9944596e80d920/js/src/vm/BytecodeUtil.cpp#2203-2211


Improves the following µ-benchmark from 205ms to 110ms:
---
function f() {
    var doit = Function("obj", `
        var q = 0;
        ${`
            try { q += obj.foo; } catch (e) {}
        `.repeat(100)}
        return q;
    `);

    var xs = [undefined, null];
    var q = 0;
    var t = dateNow();
    for (var i = 0; i < 10; ++i) {
        q += doit(xs[i&1]);
    }
    return [dateNow() - t, q];
}
print(f());
---
Attachment #8999593 - Flags: review?(tcampbell)
Attachment #8999592 - Flags: review?(tcampbell) → review+
- Change DecompileExpressionFromStack to not create a BytecodeParser when the result isn't used.
- Avoid creating zero-sized arrays in captureOffsetStack/captureOffsetStackAfter.
- And remove an unnecessary bit-field modifier. With the bit-field modifier Clang emitted |andb $0xfe,(%rbx)|, but without the modifier |movb   $0x0,(%rbx)| was used. 

Further improves the test case from comment #2 to 100ms.
Attachment #8999597 - Flags: review?(tcampbell)
Removes unused code from BytecodeUtil and fixes a style nit.
Attachment #8999598 - Flags: review?(tcampbell)
Comment on attachment 8999593 [details] [diff] [review]
bug1482846-part2-parse-once.patch

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

I was worried from bug title that you were adding a cache, but this avoiding obviously duplicate work is great.
Attachment #8999593 - Flags: review?(tcampbell) → review+
Comment on attachment 8999598 [details] [diff] [review]
bug1482846-part4-unused-code.patch

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

r++
Attachment #8999598 - Flags: review?(tcampbell) → review+
Comment on attachment 8999597 [details] [diff] [review]
bug1482846-part3-move-code-empty-array.patch

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

Nice cleanup.
Attachment #8999597 - Flags: review?(tcampbell) → review+
(In reply to Ted Campbell [:tcampbell] from comment #5)
> I was worried from bug title that you were adding a cache, [...]

Haha. :-)
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/131b2132cb79
Part 1: Mark BytecodeParser methods as const. r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/b32e7222f9dc
Part 2: Parse byte code only once when decompiling an expression. r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/88384f2cacf9
Part 3: Avoid allocating empty arrays and parsing byte code when result is not used. r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/056806c8edf3
Part 4: Remove unused code from BytecodeUtil. r=tcampbell
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.