Last Comment Bug 742163 - Clean up JSScript::jitArityCheck{Normal,Ctor}
: Clean up JSScript::jitArityCheck{Normal,Ctor}
Status: RESOLVED FIXED
[MemShrink]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Nicholas Nethercote [:njn] (on vacation until July 11)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-03 20:35 PDT by Nicholas Nethercote [:njn] (on vacation until July 11)
Modified: 2012-04-10 08:43 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (26.32 KB, patch)
2012-04-03 20:35 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
dvander: review+
Details | Diff | Review

Description Nicholas Nethercote [:njn] (on vacation until July 11) 2012-04-03 20:35:41 PDT
Created attachment 612090 [details] [diff] [review]
patch

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.
Comment 1 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-04-05 00:05:23 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/55ba87dd42e2
Comment 2 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-05 11:29:31 PDT
http://hg.mozilla.org/mozilla-central/rev/55ba87dd42e2
Comment 3 :Ms2ger 2012-04-06 04:35:11 PDT
(In reply to Nicholas Nethercote [:njn] from comment #1)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/55ba87dd42e2

This broke nomethodjit builds.
Comment 4 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-04-07 15:08:30 PDT
> This broke nomethodjit builds.

Thanks for the info.  Easter Monday is a holiday here, I'll fix it on Tuesday.
Comment 5 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-04-09 16:50:02 PDT
Unbreak --disable-methodjit:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e79f8755d65
Comment 6 :Ms2ger 2012-04-10 02:57:52 PDT
I'd rather you didn't land patches with r=Ms2ger if I haven't seen them.
Comment 7 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-10 08:43:23 PDT
https://hg.mozilla.org/mozilla-central/rev/8e79f8755d65

Note You need to log in before you can comment on or make changes to this bug.