Closed Bug 1482846 Opened 4 years ago Closed 4 years ago

Parse byte code only once in DecompileExpressionFromStack


(Core :: JavaScript Engine, enhancement)

Not set



Tracking Status
firefox63 --- fixed


(Reporter: anba, Assigned: anba)


(Blocks 1 open bug)



(4 files)

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


Improves the following µ-benchmark from 205ms to 110ms:
function f() {
    var doit = Function("obj", `
        var q = 0;
            try { q +=; } catch (e) {}
        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];
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]

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]

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

Attachment #8999598 - Flags: review?(tcampbell) → review+
Comment on attachment 8999597 [details] [diff] [review]

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
Part 1: Mark BytecodeParser methods as const. r=tcampbell
Part 2: Parse byte code only once when decompiling an expression. r=tcampbell
Part 3: Avoid allocating empty arrays and parsing byte code when result is not used. r=tcampbell
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.