Closed
Bug 742163
Opened 12 years ago
Closed 12 years ago
Clean up JSScript::jitArityCheck{Normal,Ctor}
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: n.nethercote, Assigned: n.nethercote)
Details
(Whiteboard: [MemShrink])
Attachments
(1 file)
26.32 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
Currently, JSScript has jitCtor, jitNormal, jitArityCheckNormal, jitArityCheckCtor. And JITScript has arityCheckEntry. arityCheckEntry is unused. Well, it's used very briefly in finishThisUp(), but it's written and then immediately read and never read again elsewhere, so a local variable would suffice. jitArityCheck{Ctor,Normal} serve two purposes. First, they hold the arity check entries for the corresponding JITScripts (which is odd; presumably JITScript::arityCheckEntry held that at some point in the past). Second, they indicate something extra about the JSScript -- if equal to NULL, the script hasn't been compiled, and if equal to JS_UNJITTABLE_SCRIPT, the script failed compilation. This is ugly. The attached patch cleans it up. Specifically: - JSScript::jit{Ctor,Normal} take on the tri-valued NULL/UNJITTABLE_SCRIPT/JITScript role. This means that a script marked as unjittable cannot have a JITScript, whereas it could before. To avoid accidental misuse of the tri-value (e.g. assuming that a non-NULL value is a valid JITScript) these values are now encapsulated within the new JITScriptHandle class. - script->jitArityCheck{Ctor,Normal} are moved into script->jit{Ctor,Normal}->arityCheckEntry. This shrinks JSScript by two words. In finishThisUp() and destroyChunk() we were doing an odd little normal-or-ctor? dance w.r.t. arityCheckEntry that we were weren't doing with fastEntry and argsCheckEntry. AFAICT this had exactly the same effect as just accessing arityCheckEntry directly, so I've made that change (and all the tests pass). The code generated by CallCompiler::generateFullCallStub is slightly more complex because it has to look in JITScript for the arityCheckEntry, but that's rare and so doesn't matter.
Attachment #612090 -
Flags: review?(dvander)
Updated•12 years ago
|
Attachment #612090 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 1•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/55ba87dd42e2
Comment 2•12 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/55ba87dd42e2
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Comment 3•12 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #1) > https://hg.mozilla.org/integration/mozilla-inbound/rev/55ba87dd42e2 This broke nomethodjit builds.
Assignee | ||
Comment 4•12 years ago
|
||
> This broke nomethodjit builds.
Thanks for the info. Easter Monday is a holiday here, I'll fix it on Tuesday.
Assignee | ||
Comment 5•12 years ago
|
||
Unbreak --disable-methodjit: https://hg.mozilla.org/integration/mozilla-inbound/rev/8e79f8755d65
Comment 6•12 years ago
|
||
I'd rather you didn't land patches with r=Ms2ger if I haven't seen them.
You need to log in
before you can comment on or make changes to this bug.
Description
•