1482846 - Parse byte code only once in DecompileExpressionFromStack

Status: RESOLVED FIXED

Comment 1

[André Bargull [:anba]]

Attachment: bug1482846-part1-const-methods.patch

Marks most BytecodeParser as |const| in preparation for part 2. And changes the two existing functions accepting |BytecodeParser*| to use |const BytecodeParser*| instead.

Comment 2

[André Bargull [:anba]]

Attachment: bug1482846-part2-parse-once.patch

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());
---

Comment 3

[André Bargull [:anba]]

Attachment: bug1482846-part3-move-code-empty-array.patch

- 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.

Comment 4

[André Bargull [:anba]]

Attachment: bug1482846-part4-unused-code.patch

Removes unused code from BytecodeUtil and fixes a style nit.

Comment 5

[Ted Campbell [: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.

Comment 6

[Ted Campbell [:tcampbell]]

Comment on attachment 8999598 [details] [diff] [review]
bug1482846-part4-unused-code.patch

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

r++

Comment 7

[Ted Campbell [:tcampbell]]

Comment on attachment 8999597 [details] [diff] [review]
bug1482846-part3-move-code-empty-array.patch

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

Nice cleanup.

Comment 8

[André Bargull [:anba]]

(In reply to Ted Campbell [:tcampbell] from comment #5)
> I was worried from bug title that you were adding a cache, [...]

Haha. :-)

Comment 9

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cb5d99316ca2b617ce8399a719519279725176a

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/131b2132cb79
https://hg.mozilla.org/mozilla-central/rev/b32e7222f9dc
https://hg.mozilla.org/mozilla-central/rev/88384f2cacf9
https://hg.mozilla.org/mozilla-central/rev/056806c8edf3