Closed
Bug 960040
Opened 10 years ago
Closed 10 years ago
Prune unused interfaces and code from ScriptAnalysis
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: wingo, Assigned: wingo)
Details
Attachments
(5 files, 7 obsolete files)
10.50 KB,
patch
|
wingo
:
review+
|
Details | Diff | Splinter Review |
53.75 KB,
patch
|
wingo
:
review+
|
Details | Diff | Splinter Review |
47.25 KB,
patch
|
wingo
:
review+
|
Details | Diff | Splinter Review |
6.30 KB,
patch
|
wingo
:
review+
|
Details | Diff | Splinter Review |
1.47 KB,
patch
|
Details | Diff | Splinter Review |
jsanalyze.cpp is a vestigial thing at this point, just used for some special purposes rather than a generic analysis tool. The attached patch will prune unused code and interfaces from ScriptAnalysis.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → wingo
Assignee | ||
Updated•10 years ago
|
Attachment #8360387 -
Flags: review?(jdemooij)
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8360387 -
Attachment is obsolete: true
Attachment #8360387 -
Flags: review?(jdemooij)
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8360453 -
Attachment is obsolete: true
Assignee | ||
Comment 5•10 years ago
|
||
Comment on attachment 8360452 [details] [diff] [review] Part 1: Prune unused interfaces and code from ScriptAnalysis UNREVIEWED Add "Part 1" onto the patch descrption, as I have another one that does more.
Attachment #8360452 -
Flags: review?(jdemooij)
Assignee | ||
Comment 6•10 years ago
|
||
Comment on attachment 8360454 [details] [diff] [review] Part 2: Hide more of ScriptAnalysis implementation UNREVIEWED Removes jsanalyzeinlines.h, moving its used interfaces to jsopcodeinlines.h. Moves all implementation of jsanalyze to the cpp file, just leaving the interface in the .h.
Attachment #8360454 -
Flags: review?(jdemooij)
Comment 7•10 years ago
|
||
Great work wingo! While we're on the subject, what is the current state and the eventual plan for jsanalyze and the related ScriptAnalysis/Bytecode data structures? I know we used to create a ScriptAnalysis before running any script in the interpreter and then create SSA/types before JITing. It's hard to follow the chain of ensureX() calls that end up triggering creation now, but printf'ing in analyzeBytecode/analyzeLifetimes/analyzeSSA, these don't seem to be called at any point when jitting the trivial functions I tried. In what cases are these called now? Could the remaining analyses be absorbed into baseline/ion and could ScriptAnalysis would either be removed or absorbed into IonScript or TypeScript or made local to baseline/ion compilation?
Assignee | ||
Comment 8•10 years ago
|
||
I'm working on simplifying this stuff; currently the only uses of the analysis are to determine the value of needsArgsObj. I have an uncommitted change that says: // Analysis information about a script. FIXME: At this point, the entire // purpose of this class is to compute JSScript::needsArgsObj, and to support // isReachable() in order for jsinfer.cpp:FindPreviousInnerInitializer to get // the previous opcode. For that purpose, it is completely overkill. class ScriptAnalysis so the goal is to figure out how to compute needsArgsObj without the full ScriptAnalysis; to do so I am reducing down ScriptAnalysis, and then we'll see if the reduced thing can be swapped out for somethign else... This work is a bit of a yakshave, but it's inspired by bug 956173.
Assignee | ||
Comment 9•10 years ago
|
||
If you are interested in working on this :luke, any thoughts you might have about the use of the analysis by FindPreviousInnerInitializer are welcome :) Worst case would be to somehow use the analysis in jsopcode.cpp.
Assignee | ||
Comment 10•10 years ago
|
||
Assignee | ||
Comment 11•10 years ago
|
||
Comment on attachment 8360484 [details] [diff] [review] Part 3: Refactor OOM handling UNREVIEWED The only way that the ScriptAnalysis can fail is through OOM, so refactor the error handling in the ScriptAnalysis to report OOM at allocation failure sites, to return false if a subroutine failed, and to always check return values on subroutine calls. Should simplify later things. (Jan, I selected the reviewer at random, pretty much. I can find someone else if you like :)
Attachment #8360484 -
Flags: review?(jdemooij)
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #7) > analyzeBytecode/analyzeLifetimes/analyzeSSA, these don't seem > to be called at any point when jitting the trivial functions I tried. Try anything with "arguments". I am not sure how to trigger the jsinfer.cpp case.
Comment 13•10 years ago
|
||
Ah right, arguments. We try to avoid ever creating an arguments (even in the interp) if it only flows into optimizable uses like getelem, getprop 'length', and 'apply'. I wonder if there is anything else. Because, if so, it seems like we could definitely do something about arguments.
Assignee | ||
Comment 14•10 years ago
|
||
Assignee | ||
Comment 15•10 years ago
|
||
Comment on attachment 8360523 [details] [diff] [review] Part 4: Refactor state management of analysis UNREVIEWED One last little cleanup, then I go do something productive.
Attachment #8360523 -
Flags: review?(jdemooij)
Comment 16•10 years ago
|
||
Comment on attachment 8360452 [details] [diff] [review] Part 1: Prune unused interfaces and code from ScriptAnalysis UNREVIEWED Review of attachment 8360452 [details] [diff] [review]: ----------------------------------------------------------------- Nice, thanks. ::: js/src/jsanalyze.h @@ +799,5 @@ > public: > #ifdef DEBUG > void assertMatchingDebugMode(); > + void assertMatchingStackDepthAtOffset(uint32_t offset, uint32_t stackDepth) { > + JS_ASSERT_IF(maybeCode(offset), getCode(offset).stackDepth == stackDepth); Nit: indent with 2 more spaces.
Attachment #8360452 -
Flags: review?(jdemooij) → review+
Updated•10 years ago
|
Attachment #8360454 -
Flags: review?(jdemooij) → review+
Updated•10 years ago
|
Attachment #8360484 -
Flags: review?(jdemooij) → review+
Comment 17•10 years ago
|
||
Comment on attachment 8360523 [details] [diff] [review] Part 4: Refactor state management of analysis UNREVIEWED Review of attachment 8360523 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsinferinlines.h @@ +1426,5 @@ > if (!ensureHasTypes(cx)) > return false; > if (!hasAnalysis() && !makeAnalysis(cx)) > return false; > + JS_ASSERT(hasAnalysis()); Hm, I wonder if we can get rid of the types->analysis pointer now. Then we don't have to call ensureHasTypes and we can just allocate ScriptAnalysis on the stack...
Attachment #8360523 -
Flags: review?(jdemooij) → review+
Comment 18•10 years ago
|
||
Great work! Yes ScriptAnalysis is only used for the arguments-analysis these days. It seems overkill to have a complete SSA+lifetime analysis for that though, maybe we could do the arguments optimization as part of Ion somehow. Maybe with a very simple bytecode analysis for Baseline to catch the simple arguments[i]/arguments.length cases.
Assignee | ||
Comment 19•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8360452 -
Attachment is obsolete: true
Assignee | ||
Comment 20•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8360454 -
Attachment is obsolete: true
Assignee | ||
Comment 21•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8362930 -
Attachment is obsolete: true
Assignee | ||
Comment 22•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8360484 -
Attachment is obsolete: true
Assignee | ||
Comment 23•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8360523 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8362936 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8362935 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8362931 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8362891 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 24•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/592066fa82d2 https://hg.mozilla.org/integration/mozilla-inbound/rev/697fcc313a0d https://hg.mozilla.org/integration/mozilla-inbound/rev/98baa9ac63f6 https://hg.mozilla.org/integration/mozilla-inbound/rev/a7ba0d202c1a
Keywords: checkin-needed
Assignee | ||
Comment 25•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/592066fa82d2 https://hg.mozilla.org/mozilla-central/rev/697fcc313a0d https://hg.mozilla.org/mozilla-central/rev/98baa9ac63f6 https://hg.mozilla.org/mozilla-central/rev/a7ba0d202c1a https://hg.mozilla.org/mozilla-central/rev/83be4731a1a1
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in
before you can comment on or make changes to this bug.
Description
•