Closed
Bug 1482846
Opened 4 years ago
Closed 4 years ago
Parse byte code only once in DecompileExpressionFromStack
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: anba, Assigned: anba)
References
(Blocks 1 open bug)
Details
Attachments
(4 files)
6.73 KB,
patch
|
tcampbell
:
review+
|
Details | Diff | Splinter Review |
6.71 KB,
patch
|
tcampbell
:
review+
|
Details | Diff | Splinter Review |
4.70 KB,
patch
|
tcampbell
:
review+
|
Details | Diff | Splinter Review |
4.85 KB,
patch
|
tcampbell
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•4 years ago
|
||
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)
Assignee | ||
Comment 2•4 years ago
|
||
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)
Updated•4 years ago
|
Attachment #8999592 -
Flags: review?(tcampbell) → review+
Assignee | ||
Comment 3•4 years ago
|
||
- 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)
Assignee | ||
Comment 4•4 years ago
|
||
Removes unused code from BytecodeUtil and fixes a style nit.
Attachment #8999598 -
Flags: review?(tcampbell)
Comment 5•4 years ago
|
||
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 6•4 years ago
|
||
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 7•4 years ago
|
||
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+
Assignee | ||
Comment 8•4 years ago
|
||
(In reply to Ted Campbell [:tcampbell] from comment #5) > I was worried from bug title that you were adding a cache, [...] Haha. :-)
Assignee | ||
Comment 9•4 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cb5d99316ca2b617ce8399a719519279725176a
Keywords: checkin-needed
Comment 10•4 years ago
|
||
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
Comment 11•4 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in
before you can comment on or make changes to this bug.
Description
•