Closed Bug 528375 Opened 10 years ago Closed 8 years ago

Add jit-min and jit-max compilation policy

Categories

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Q4 11 - Anza

People

(Reporter: rreitmai, Assigned: rreitmai)

References

Details

Attachments

(2 files, 4 obsolete files)

Attached patch rev1 - looking for comments (obsolete) — Splinter Review
Currently the JIT uses MethodInfo.suggestInterp() to decide whether to compile a method or not.   

It is thus currently difficult to experiment with different jit'ing policies as the call is *baked* into MethodInfo.verify(). 

The attached patch introduces a jit policy manager external from MethodInfo which centralizes the decision about which methods to jit.

Currently two policies are supported; 
(1) for release builds, a call is made to suggestInterp(), effectively retraining the existing behaviour.
(2) for verbose builds, the policy is (1) unless the command-line option -Djitonly is provided, in which case compiled methods will be restricted to those identified by the option.
Attachment #412108 - Flags: review?(edwsmith)
Comment on attachment 412108 [details] [diff] [review]
rev1 - looking for comments 

(just for background, bug 487482 also proposes a new policy manager that encapsulates more of the decision making)

This patch is a decent start.  Ultimately the goal is to have a modular execution manager that owns the execution mechanism (choice of interp, jit, mixed-mode, verifyall(eager), etc) and internaly implements either a fixed policy (e.g. for early mode or wordcode-only or abc-only interps) or a configurable policy (jit/interp hybrids).

* MethodInfo.cpp:319  the JitMgr->shouldCompile() call should also include the core->isJitEnabled() check.

* check indentation, several chunks of new code don't look like they match the surrounding code.  normal formatting is a space between if/while and (.  (e.g. CodegenLIR.cpp:5058.

* CodegenLIR.h:127: no method bodies in header file please, they go in CodegenLIR-inlines.h.  for shouldCompile(), since there are two versions in two different files, please add comments pointing back and forth.  A comment explaining what the policy implements is also good for the verbose version of shouldCompile.

* the limit on the # of names should be defined as a constant.  in this patch, its hardcoded at CodegenLIR.h:135 and CodegenLIR.cpp:5058.

* we still need to support jit-crash bisecting by enabling AVMPLUS_JITMAX on release builds, independent from AVMPLUS_VERBOSE
Attachment #412108 - Flags: review?(edwsmith) → review-
Attached patch ver 2 - rebase (obsolete) — Splinter Review
no real changes from existing patch just rebased since I needed to use it.  Will address Eds concerns in next patch.
Attachment #412108 - Attachment is obsolete: true
Flags: flashplayer-qrb+
Priority: -- → P2
Target Milestone: --- → flash10.2
Depends on: 563119
Blocks: OSR
Summary: Mechanism for JIT policy management → Add jit-min and jit-max compilation policy
No longer blocks: OSR
Component: Virtual Machine → JIT Compiler (NanoJIT)
Code needs to be re-formed in the context of bug 570803
Assignee: nobody → rreitmai
Flags: flashplayer-qrb+ → flashplayer-qrb?
Is this still in play?
Very low priority but highly valuable for jit debugging.  So yes, lets leave it open but it doesn't make sense to schedule it for a milestone.
Target Milestone: flash10.x-Serrano → Future
Priority: P2 → --
Attached patch wip (obsolete) — Splinter Review
First draft of a policy override mechanism useful during debugging.  On the command-line specifying -policy enables per method selection of interp/jit preference.  E.g. "-policy jit=25,interp=Function/prime_val.as\$1:prime,jit=40-"
means to jit method #25 , interp function prime() , and jit method #'s 40 and above.  Used with -verbose=execpolicy one can fine tune the treatment of methods exactly.

A couple of remaining issues:
(1) method ids/sequence #s are based on call count to shouldJIT(), which is accurate for the current jit due to single call per method, but in a world where recompilation is possible this does not hold true.
(2) method names require exact string matches.  It would be nice to support a regex rule but more work is needed in order to understand how best to do so.
Attachment #418938 - Attachment is obsolete: true
Attachment #524018 - Flags: feedback?(edwsmith)
Attached patch rev 1 (obsolete) — Splinter Review
Revised patch;
(1) Resolves the method id issue by using a unique value per pool that is added to the method id.
(2) Adds documentation

Of note, this code is enabled in release builds.  

This is because we are planning to remove debugger build configurations, so it doesn't make sense to isolate it, considering that it will eventually be exposed.

In terms of overhead, if no policy options are specified the run-time cost of this code is a single check of the config flag 'policyRulesString' per method on each shouldJit() query, along with an integer increment and call per PoolObject parsed.

Moving ed from feedback to sr.
Attachment #524018 - Attachment is obsolete: true
Attachment #524084 - Flags: superreview?(edwsmith)
Attachment #524084 - Flags: review?(kpalacz)
Attachment #524018 - Flags: feedback?(edwsmith)
Comment on attachment 524084 [details] [diff] [review]
rev 1

Useful! As a future extension, it would be nice to be able to drop the namespace  part of MethodInfo name when typing on the command line.

Here's one suggestion worth addressing: BaseExecMrg::prepPolicyRules() is called lazily and asserts on invalid user input. I'd rather have it run eagerly at startup and die gracefully (if possible). Lazy initialization without synchronization tends to cause problems with concurrency, in particular  the background compiler could theoretically run into trouble here.

The rest are nits, feel free to ignore.

* I think I'd prefer the PoolObject id and MethodInfo unique method id logic to be more stereotyped for easy recognition.
assignPoolAnId() -  an article in a method name name looks unusual, maybe  assignUniquePoolId() or assignPoolId() instead?
It's not obvious what PoolObject::assignMethodIdBase() returns, it could return void and PoolObject::methodCount() could be invoked in its caller instead.
Also, AvmCore::nextUniquePoolId is propagated  to PoolObject using PoolObject::assignMethodIdBase(), the names could be more unified.
I would rename AvmCore::nextUniquePoolId according to what is really is, i.e.,  to something like currentMethodInfoCount, and then define
void AvmCore:assignPoolId(PoolObject* pool)
{
     pool->assignUniqueId(this->currentMethodInfoCount + 1);
     curentMethodInfoCount += pool->methodCount();
}

I'm not sure I understand why nextUniquePoolId is initialized to 1, is it sot that _methodIdBase != 0 can be used to detect an error condition in its setter? I'd bite the bullet and allow unique pool ids to be signed ints, and use -1 to detect an error condition instead (this would reinforce the notion that unique method ids are derived from method ids).

AvmCore::nextUniquePoolId doesn't use our convention for private fields (neither do neighboring fields in AvmCore, but if we're trying to be consistent ...).

* BaseExecMgr::parseDigit() - rather surprising to find such a method in the interface of BaseExecMgr, could we have a static function instead?

* I'd prefer initializing pointer fields with NULL instead of 0 for better readability (config.policyRulesString, BaseExecManager::_ruleSet).

* some diff lines are whitespace only, not sure if we care about that.
Attachment #524084 - Flags: review?(kpalacz) → review+
Addressed all concerns raised in comment 8.

Asking for re-review since added support for regular expressions and factored out the method recognition code into a separate class MethodRecognizer so that we'll be able to re-use the logic for -Dverbose method specific output (see bug 542913).
Attachment #524084 - Attachment is obsolete: true
Attachment #528392 - Flags: superreview?(edwsmith)
Attachment #528392 - Flags: review?(kpalacz)
Attachment #524084 - Flags: superreview?(edwsmith)
Blocks: 542913
Comment on attachment 528392 [details] [diff] [review]
update to patch per 1st review comments

Review of attachment 528392 [details] [diff] [review]:
-----------------------------------------------------------------

Some nits/suggestions:

::: core/MethodInfo.cpp
@@ +1108,5 @@
> +            delete _pattern;
> +
> +            uint32_t errLen = (errStr == NULL) ? 0 : VMPI_strlen(errStr);
> +            const char* msg = "*** REGULAR EXPRESSION PARSE ERROR *** : ";
> +            const char* msgcont = " in : ";

Why not use VMPI_snprintf() here?

@@ +1109,5 @@
> +
> +            uint32_t errLen = (errStr == NULL) ? 0 : VMPI_strlen(errStr);
> +            const char* msg = "*** REGULAR EXPRESSION PARSE ERROR *** : ";
> +            const char* msgcont = " in : ";
> +            str = new char[VMPI_strlen(msg) + errLen + sizeof(msgcont) + length + 1];

Shouldn't sizeof(msgcont)  be VMPI_strlen(msgcont) ?

@@ +1111,5 @@
> +            const char* msg = "*** REGULAR EXPRESSION PARSE ERROR *** : ";
> +            const char* msgcont = " in : ";
> +            str = new char[VMPI_strlen(msg) + errLen + sizeof(msgcont) + length + 1];
> +            VMPI_strcpy(str, msg);
> +            (errLen) ? VMPI_strcat(str, errStr) : 0;

if () would be more legible here.

::: core/exec.cpp
@@ -312,3 +314,3 @@
> >  # ifdef VMCFG_NANOJIT
> > -    if (m->pool()->isVerbose(VB_execpolicy)) // Currently shouldn't print "unknown", accounting for code evolution.
> > +    if (m->pool()->isVerbose(VB_execpolicy))
> > -        core->console << "execpolicy interp " << m << (shouldJit(m, ms) ? " unknown\n" : " jit-policy\n");
> > +        core->console << "execpolicy interp (" << (uint32_t)m->unique_method_id() << ") " << m << " jit-available\n";

> > -        core->console << "execpolicy interp " << m << (shouldJit(m, ms) ? " unknown\n" : " jit-policy\n");
> > +        core->console << "execpolicy interp (" << (uint32_t)m->unique_method_id() << ") " << m << " jit-available\n";

> > -        core->console << "execpolicy interp " << m << (shouldJit(m, ms) ? " unknown\n" : " jit-policy\n");
> > +        core->console << "execpolicy interp (" << (uint32_t)m->unique_method_id() << ") " << m << " jit-available\n";

unique_method_id() already returns uint32_t, right?

::: shell/avmshell.cpp
@@ +1245,5 @@
>      #ifdef AVMPLUS_ARM
>          AvmLog("          [-Darm_arch N]  nanojit assumes ARMvN architecture (default=5)\n");
>          AvmLog("          [-Darm_vfp]     nanojit uses VFP rather than SoftFloat\n");
>      #endif
> +        AvmLog("          [-policy [{interp|jit}={id|name|?regex}]+ ]\n");

A non-wildcard regex prefix would be somewhat easier to type (perhaps /regex/ or just /regex instead of ?regex).
Attachment #528392 - Flags: review?(kpalacz) → review+
(In reply to comment #10)
> Why not use VMPI_snprintf() here?
> 
Possible, but when I tried the code looked even uglier.

> 
> Shouldn't sizeof(msgcont)  be VMPI_strlen(msgcont) ?

Good catch!

> unique_method_id() already returns uint32_t, right?
> A non-wildcard regex prefix would be somewhat easier to type (perhaps /regex/ 

Both fixed.  The parser now expects '/regex/' form.
Flags: flashplayer-qrb?
Flags: flashplayer-qrb+
Flags: flashplayer-injection-
Flags: flashplayer-bug-
Priority: -- → P3
Target Milestone: Future → Q4 11 - Anza
Comment on attachment 528392 [details] [diff] [review]
update to patch per 1st review comments

I didn't scan the code for bugs, but it seems the patch is missing any kind of testing (self tests? as3 tests?) and there's also no developer documentation, and for something like this, there really should be.

Is it intentional that all the new code is not guarded, so it will work in release mode?  if so, is there a following bug that will create a player api for it? if not, add VMCFG_JITPOLICY or something (on always in shell, off always in player?)

if this is exposed in release builds, what due-diligence have we done, vetting the use of user-controllled regex strings?
Attachment #528392 - Flags: superreview?(edwsmith) → superreview+
(In reply to comment #12)
> also no developer documentation

Expanded on the comments making the intent clearer, if that's what you were thinking of.

> Is it intentional that all the new code is not guarded

Added VMCFG_COMPILEPOLICY which is enabled for shell builds.
changeset: 6311:df58424e39d9
user:      Rick Reitmaier <rreitmai@adobe.com>
summary:   Bug 528375 - Add jit-min and jit-max compilation policy (r+kpalacz,edwsmith)

http://hg.mozilla.org/tamarin-redux/rev/df58424e39d9
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Also removed redundant jitmin/max code via http://hg.mozilla.org/tamarin-redux/rev/882e61df1abd
(In reply to comment #14)
> changeset: 6311:df58424e39d9
> user:      Rick Reitmaier <rreitmai@adobe.com>
> summary:   Bug 528375 - Add jit-min and jit-max compilation policy
> (r+kpalacz,edwsmith)
> 
> http://hg.mozilla.org/tamarin-redux/rev/df58424e39d9

This change breaks the WORDCODE compilation.

../configure.py --enable-wordcode-interp 

../core/avmfeatures.h:856:6: error: #error "AVMFEATURE_JIT is required for AVMFEATURE_COMPILEPOLICY"


When AVMFEATURE_WORDCODE_INTERP is enabled, AVMFEATURE_JIT must NOT exist, but the AVMFEATURE_COMPILEPOLICY which is enabled by default is required.

Would the correct change here be that we need to modify how we build the wordcode shell to also pass --disable-compilepolicy, since I am not sure there is a way to express this in the avmfeatures.as
I think that's the correct fix.   

Also I just added :
    <precludes>  AVMFEATURE_COMPILEPOLICY </precludes>
to the WORDCODE_INTERP entry so that the error output hints that you need to disable compilepolicy.

e.g.
../core/avmfeatures.h:856:6: error: #error "AVMFEATURE_JIT is required for AVMFEATURE_COMPILEPOLICY"
../core/avmfeatures.h:874:6: error: #error "AVMFEATURE_COMPILEPOLICY is precluded for AVMFEATURE_WORDCODE_INTERP"
If you enable AVMFEATURE_COMPILEPOLICY in avmshell-features.h only if JIT is enabled, then the existing "requires" constraint is all you need, and you wouldn't need any extra build flags.
Attached patch features patchSplinter Review
By default, only enable the AVMFEATURE_COMPILEPOLICY if AVMFEATURE_JIT is enabled.

This also contains the generated change to core/avmfeatures.h from change 6322 that was not included.
Attachment #533950 - Flags: review?(rreitmai)
Attachment #533950 - Flags: review?(rreitmai) → review+
changeset: 6324:dc8c62042eb2
user:      Brent Baker <brbaker@adobe.com>
summary:   Bug 528375: By default, only enable the AVMFEATURE_COMPILEPOLICY if AVMFEATURE_JIT is enabled. (r=rreitmai)

http://hg.mozilla.org/tamarin-redux/rev/dc8c62042eb2
You need to log in before you can comment on or make changes to this bug.