Closed
Bug 733950
Opened 12 years ago
Closed 12 years ago
create arguments object eagerly
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: luke, Assigned: luke)
References
Details
Attachments
(4 files, 1 obsolete file)
132.58 KB,
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
1.11 KB,
patch
|
Details | Diff | Splinter Review | |
19.92 KB,
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
1.75 KB,
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
These patches create arguments objects eagerly or not at all. They are a prerequisite to bug 659577. I'd like to land it now that I think we can assume bug 718022 stuck.
Assignee | ||
Updated•12 years ago
|
Assignee: ashuk → general
Component: Java APIs for DOM → JavaScript Engine
QA Contact: dom-apis → general
Assignee | ||
Comment 1•12 years ago
|
||
This patch creates arguments eagerly. It disables the important fun.apply optimization; a later patch adds it back.
Assignee | ||
Comment 2•12 years ago
|
||
I was weirded out by what it would mean if we had an ARGUMENTS opcode but no Bytecode so I just asserted it doesn't happen... is this valid?
Attachment #603915 -
Flags: review?(bhackett1024)
Assignee | ||
Comment 3•12 years ago
|
||
With this patch, we go back to our previous optimization level: zero arguments objects are created in v8.
Attachment #603916 -
Flags: review?(bhackett1024)
Assignee | ||
Comment 5•12 years ago
|
||
Oops, wrong version
Attachment #603916 -
Attachment is obsolete: true
Attachment #603946 -
Flags: review?(bhackett1024)
Attachment #603916 -
Flags: review?(bhackett1024)
Comment 6•12 years ago
|
||
Comment on attachment 603914 [details] [diff] [review] eager arguments Review of attachment 603914 [details] [diff] [review]: ----------------------------------------------------------------- Nice! Sorry for the late review. ::: js/src/jsscript.h @@ +422,5 @@ > #endif > bool callDestroyHook:1;/* need to call destroy hook */ > > + /* > + * TODO: describe Needs a comment.
Attachment #603914 -
Flags: review?(bhackett1024) → review+
Comment 7•12 years ago
|
||
Comment on attachment 603915 [details] [diff] [review] rm a check This check for 'code' needs to be left in. In general, the Bytecode for an opcode will not be present if the opcode is unreachable in the script's CFG, e.g. 'function foo() { return 0; return arguments; }'. The SSA code will get crash if it is fed SSA values referring to unreachable opcodes. It would be good to add a comment describing why the check is here.
Attachment #603915 -
Flags: review?(bhackett1024)
Comment 8•12 years ago
|
||
Comment on attachment 603914 [details] [diff] [review] eager arguments Also, this patch looks like it's supposed to have ArgumentsObject.cpp. Can you attach a separate patch with that file for review?
Comment 9•12 years ago
|
||
Comment on attachment 603946 [details] [diff] [review] reoptimize f.apply(arguments) Review of attachment 603946 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsscript.cpp @@ +1988,5 @@ > + > + if (hasAnalysis() && analysis()->ranInference()) { > + types::AutoEnterTypeInference enter(cx); > + for (unsigned off = 0; off < length; off += GetBytecodeLength(code + off)) { > + if (code[off] == JSOP_ARGUMENTS) { This should also check that analysis->maybeCode(off) != NULL.
Attachment #603946 -
Flags: review?(bhackett1024) → review+
Assignee | ||
Comment 10•12 years ago
|
||
(In reply to Brian Hackett (:bhackett) from comment #8) Thanks for the reviews. Diff is making it look weird, but what I did (to preserve hg history and minimize rebase pain) was to "hg cp jsfun.cpp vm/ArgumentsObject.cpp" and then delete all the not-arguments-object stuff. Diff shows it as two separate edits to jsfun.cpp.
Assignee | ||
Comment 11•12 years ago
|
||
While I was waiting for the dependencies to get landed I noticed I'm not actually ensuring the !heavyweight assertion in JSScript::applySpeculationFailed. I'll fold the fix in to the prev patch, but here's the diff.
Attachment #605615 -
Flags: review?(bhackett1024)
Updated•12 years ago
|
Attachment #605615 -
Flags: review?(bhackett1024) → review+
Assignee | ||
Comment 12•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2107141265f https://hg.mozilla.org/integration/mozilla-inbound/rev/8f3b804118ef and then I noticed I cited the wrong bug so I relanded with the correct bug # https://hg.mozilla.org/integration/mozilla-inbound/rev/85bef04d1258 https://hg.mozilla.org/integration/mozilla-inbound/rev/e2144e6ee774
Comment 13•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/85bef04d1258 https://hg.mozilla.org/mozilla-central/rev/e2144e6ee774
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in
before you can comment on or make changes to this bug.
Description
•