563119 - Group all execution management code into ExecMgr module

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P1)

Description

[Edwin Smith]

Attachment: create ExecMgr interface and single implementation: SimpleExecMgr

Experimental refactoring to isolate the code that manages execution, including:

* management of the code pointers in MethodEnv and MethodInfo
* installation of verify-on-first-call trampolines
* jit policy (what to jit and when to jit it)
* installation of interpreter or jit trampolines
* IMT thunk generation and invocation
* verifyall and verifyonly modes (hooks, queues, etc)

This first draft collects the various bits of code from around the system and puts them in one place.  Behavior is unmodified:  the same trampolines are used in the same situations, and the same policies are implemented for all modes.

Work in progress is here:
http://hg.mozilla.org/users/edwsmith_adobe.com/jitpolicy/rev/execmgr

new files:
exec.h              ExecMgr interface and SimpleExecMgr implementation
exec.cpp            SimpleExecMgr implementation
exec-imt.cpp        IMT management code and thunks
exec-verifyall.cpp  extra code for verifyall and verifyonly modes

Posting to get early feedback on direction, naming, factoring opinions etc.

Where this is headed:
 - separate execution policy from mechanism
 - support for new jit policies:  lazy compilation, OSR, background compilation
 - isolate the code for jit->jit invocation so we can tailor
   the calling conventions and generate trampolines
 - (maybe) separate ExecMgr implementations for verifyall-mode,
   interp-only mode, etc

Comment 2

[Edwin Smith]

Attachment: Replace SUGGEST_INTERP flag with STATIC_INIT flag

Better to record facts about methods, and make localized decisions about whether to jit them, than to embed a jit policy flag in the parser.  Changing this flag is one step in that direction.

Comment 3

[Edwin Smith]

Comment on attachment 443094 [details] [diff] [review]
Replace SUGGEST_INTERP flag with STATIC_INIT flag

pushed
http://hg.mozilla.org/tamarin-redux/rev/68eadda8f206

Comment 4

[Edwin Smith]

Attachment: Remove method address formatting code from debugger

Comment 5

[Mike Morearty]

Comment on attachment 444414 [details] [diff] [review]
Remove method address formatting code from debugger

Reviewed, looks good to me.

Comment 6

[Edwin Smith]

Comment on attachment 444414 [details] [diff] [review]
Remove method address formatting code from debugger

TR: http://hg.mozilla.org/tamarin-redux/rev/15151e4dfe5d

Comment 7

[Edwin Smith]

Attachment: (v2) create ExecMgr interface and single implementation: BaseExecMgr

feedback-ready patch.  This has stabilized in the last couple weeks, and I have implemented a few other enhancements on top of it to make sure the basic factoring is about right (OSR and some optimizations to how native method thunks are installed).

Again, the patch reflects no changes in execution policy, just code reorganization and cleanup.

some questions to think about
* exception logic is not encapsulated, but it should it be?  It doesn't look very hard to tailor the exception mechanism to the method containing the catch block; dispatch could be virtualized pretty easily, I think.

* BaseExecMgr is one big blob, still.  This patch groups all the code, but it probably needs further factoring.  good enough to land or keep going?

* any naming concerns or objections?  In particular, should ExecMgr get dedicated api's for Function.apply and Function.call?

* Take a look at how verifyOnCall(), verifyMethod(), verifyNative(), verifyInterp(), and verifyJit() are factored.  It splits up the mess that was in MethodInfo::verify(), particularly the need for placement-new in setting up CodeWriter pipeline elements.  It should get the cleanup logic right, still, but if you see something that makes you think otherwise, let me know.

Comment 8

[Steven Johnson]

Comment on attachment 448599 [details] [diff] [review]
(v2) create ExecMgr interface and single implementation: BaseExecMgr

Generally good, nits:

-- Probably sensible to land in one big blob initially; this is too large to manage all the refactoring in one go.

-- Is there any way (aside from code reviews) we can help ensure that future changes continue to get bottlenecked into ExecMgr?

-- new/delete -> mmfx_new/mmfx_delete, I think

-- GprImtThunkProcRetType uses "int" and "uint32", please use C99 ints (ditto various other places in ExecMgr)

-- is the change from "offsetof()" to "cast a null pointer" simply to avoid compiler warnings? 

-- nit:

    // return true on success, false on failure (frame size too large)
    GprMethodProc CodegenLIR::emitMD()

Looks like a misleading comment, unless "GprMethodProc" is an alias for "bool"

-- no concern over Function.apply and Function.call, these are weird beasts anyway

-- Verifier now takes a MethodSignature but doesn't appear to use it for anything.

Comment 9

[Edwin Smith]

(In reply to comment #8)
> (From update of attachment 448599 [details] [diff] [review])

> -- Is there any way (aside from code reviews) we can help ensure that future
> changes continue to get bottlenecked into ExecMgr?

I'll add comments to exec.h, other suggestions welcome.

> -- new/delete -> mmfx_new/mmfx_delete, I think

where?

> -- GprImtThunkProcRetType uses "int" and "uint32", please use C99 ints (ditto
> various other places in ExecMgr)

I'll review 'int' uses for C99 but no promises; sometimes I prefer plain 'int' for api's.  for the other cases, i'll find them and submit a separate cleanup patch.  We really should just finish the C99 type refactoring in TR+FRR before the fresh paint dries on the release.

> -- is the change from "offsetof()" to "cast a null pointer" simply to avoid
> compiler warnings? 

entries[] moved into a nested structure, so it seemed like a good idea to just let the C++ compile compute the offset of the array field within the nested imt structure, within VTable, instead of doing it all explicitly.  But it doesn't seem important now.  I'll revert.

> Looks like a misleading comment, unless "GprMethodProc" is an alias for "bool"

fixed

> -- Verifier now takes a MethodSignature but doesn't appear to use it for
> anything.

It is used in the Verifier() constructor to get abc body parameters (local_count, etc) as well as later during verification to access the method signature.  I passed this in since its readily available in all callers and avoids hitting the MethodInfo weak ref redundantly.

Comment 10

[Steven Johnson]

(In reply to comment #9)
> > -- new/delete -> mmfx_new/mmfx_delete, I think
> 
> where?

creating ExecMgr in AvmCore
 
> I'll review 'int' uses for C99 but no promises; sometimes I prefer plain 'int'
> for api's.

This requires further discussion (probably off-scope of this bug) but I think such an assertion requires some justification...

> It is used in the Verifier() constructor to get abc body parameters
> (local_count, etc) as well as later during verification to access the method
> signature.  I passed this in since its readily available in all callers and
> avoids hitting the MethodInfo weak ref redundantly.

OK, I must have overlooked it.

Comment 11

[Simon Wilkinson]

(In reply to comment #7)
Something that has been bothering me:

Currently Runmode is a global enum referenced in AvmCore, Config, exec-jit and ShellCoreSettings (and hardcoded into the commandline args). I'd say this implies that all the Runmodes are applicable to all ExecMgrs. For RM_jit_all and RM_interp_all this should be the case, and should produce the expected behavior. But 'RM_mixed' may be insufficient to describe everything other ExecMgrs offer. For BaseExecMgr it means 'interpret functions/methods we identify at load-time as being run once, and jit everything else'. For other ExecMgrs will it just mean 'use the mechanism/policy that isn't RM_interp_all or RM_jit_all'?

So it's a bit of a hand-wavy suggestion, but I think the ExecMgr implementing class should define the available modes. Maybe RM_mixed can then be renamed to its truer semantics of 'RM_execmgr_default'.

Comment 12

[Edwin Smith]

I'd be fine with that, rename in the short term.

nd I too have been bothered by the option situation.  Really, the execmgr implementation will have its own implementation specific options, which are processed in ShellCore, AvmCore, held in Config, and only used by the module implementation.  

i dont have a clean solution to propose.  abstractly we'd like modules to expose options, and the option parser to parse them and pass them modules.  i've seen frameworks that do this with java reflection but it all seems like overkill here.  as we roll on i'm sure we'll add new runmodes and/or tuning parameters, which will need to be ignored or rejected in some configs.

a strategy that has worked ok so far is to just hardcode the extra options, and be forgiving about them when it makes sense.  (e.g. -Dinterp is ignored in an interpreter-only build configuration).  not well encapsulated but not too horrible either.

Comment 13

[Lars T Hansen]

Comment on attachment 448599 [details] [diff] [review]
(v2) create ExecMgr interface and single implementation: BaseExecMgr

Why are Interpreter.cpp and WordcodeEmitter.cpp including Interpreter.h?  avmplus.h includes it for them.

Are you sure removing the exception machinery in Verifier::verify will not introduce storage leaks as exceptions throw past stack-allocated storage, notably the CodeWriter?

The cliche in CodegenLIR.cpp, "&((VTable*)0)->imt.entries[index])", is strictly speaking illegal, I think, and will probably get you into trouble at some point.

Lots of documentation missing in the exec code.

Lots of uncapitalized unpunctuated comments in the exec code.

Seems like nice refactoring overall.

Comment 14

[Edwin Smith]

(In reply to comment #10)
> (In reply to comment #9)
> > > -- new/delete -> mmfx_new/mmfx_delete, I think
> > 
> > where?
> 
> creating ExecMgr in AvmCore

I think it's okay as-is, I want gc allocation, not fixed allocation:

    exec = new (gc) BaseExecMgr(this);

Comment 15

[Edwin Smith]

(In reply to comment #13)
> (From update of attachment 448599 [details] [diff] [review])

> Why are Interpreter.cpp and WordcodeEmitter.cpp including Interpreter.h? 
> avmplus.h includes it for them.

Not any more, interpreter.h is only part of the BaseExecMgr implementation and
so is only included by friends.  This mirrors the situation with CodegenLIR.h.

> Are you sure removing the exception machinery in Verifier::verify will not
> introduce storage leaks as exceptions throw past stack-allocated storage,
> notably the CodeWriter?

reasonably sure yes, but point noted for extra scrutiny/testing.

> The cliche in CodegenLIR.cpp, "&((VTable*)0)->imt.entries[index])", is strictly
> speaking illegal, I think, and will probably get you into trouble at some
> point.

I have reverted.

> Lots of documentation missing in the exec code.

Happy to add lots of documentation :-).  speak up if theres specific points you'd like to have covered; a newcomer's perspective is helpful here since i'm not one.

> Lots of uncapitalized unpunctuated comments in the exec code.

willfix

Comment 16

[Steven Johnson]

(In reply to comment #13)
> The cliche in CodegenLIR.cpp, "&((VTable*)0)->imt.entries[index])", is strictly
> speaking illegal, I think, and will probably get you into trouble at some
> point.

It's certainly ugly, but how is it illegal?

Comment 17

[William Maddox]

The expression dereferences a null pointer.  I don't think it matters, formally
speaking, that only an address is being computed and no actual load or store is
taking place.  The standard is written in to accomodate implementations in
which pointers are not offsets into a linear address space, and in which the
actual representation of the null pointer internally is not zero, so it's not
correct to assume that this expression computes an offset as you might naively
expect it to on the platforms we normally deal with.  A compiler is free to
exploit the language rules for optimization purposes in ways that are
counterintuitve with respect to the naive implementation model.  I haven't
thought about whether there are in fact any plausible optimizations which would
exploit this case.

Here's a case I once saw in real code:  A pointer was being tested for NULL
after it had been read.  The read succeeded, and the garbage value was ignored
following the failing null check.  Then, the a new release of the compiler came
out that assumed that any expression that had been dereferenced could not be
null, because it's illegal to dereference a null pointer, and the standard says
the effect is undefined if you do.  The null check then always failed, and so
did the program.

Comment 18

[Edwin Smith]

(In reply to comment #15)
> (In reply to comment #13)
> > Are you sure removing the exception machinery in Verifier::verify will not
> > introduce storage leaks as exceptions throw past stack-allocated storage,
> > notably the CodeWriter?
> 
> reasonably sure yes, but point noted for extra scrutiny/testing.

I've satisfied myself with further testing, which includes several tests from fuzzing that exposed memory leaks (the tests are in the regression suite now).  I added a comment warning about introducing locally allocated CodeWriters in Verifier::verify().

I think I'll factor this cleanup out of the patch and submit it separately.  Even in the TR, there's no purpose for the TRY/CATCH in Verifier::verify.. there is no cleanup code in its catch block, the catch block just re-throws, and the try/catch is marked as "rethrow" so the debugger isn't even interested in it.

Comment 19

[Edwin Smith]

Attachment: Remove unnecessary TRY/CATCH in Verifier::verify()

This cleanup is split out from the big patch since its unrelated.  I also removed the local var CodeWriter* coder, and added one in VerifyBlock; its meant to expedite all the many references from the verifier switch() block, but it got left behind in the verifier upgrade refactoring.

Comment 20

[Edwin Smith]

Comment on attachment 449306 [details] [diff] [review]
Remove unnecessary TRY/CATCH in Verifier::verify()

pushed: http://hg.mozilla.org/tamarin-redux/rev/68d4b50254c7

Comment 21

[Edwin Smith]

Attachment: (v3) Create ExecMgr interface and single implementation: BaseExecMgr

Changes since previous patch:

Added lots of comments, fixed comment wording & puctuation, and cleaned up code.  Also rebased to latest tamarin-redux and incorporated changes from feedback, and (via sandbox) fixed build issues on non-mac platforms.

I carefully reviewed asm output and single-stepped through a few key interpreter call paths to ensure tail calls worked as before (they do).  I also tested the various ways verifier exceptions can trigger CodeWriter pipeline cleanup to make sure there are no leaks (there arent).  Existing regressions actually cover this well now.

I decided to leave "RM_mixed" written as-is, since the current name of the enum implies "default hybrid policy for whatever execmgr is used" about as well as a new, renamed one.  If we get a more elaborate way to handle module-implementation defined settings, we can clean this up even better.

I refactored resolveImt() and dispatchImt() so the trampolines are minimized and the real work is delegated to C++ helpers, anticipating future nirvana where all JIT abi trampolines are JIT generated.  For dispatchImt(), the helper is REALLY_INLINE.

Comment 22

[Edwin Smith]

not said, but implied:  I think this is ready to land, and followup work is noted in dependent bugs.

Comment 23

[Lars T Hansen]

There's a strange new file here, 'patches' on the top level.

Comment 24

[Lars T Hansen]

Comment on attachment 449676 [details] [diff] [review]
(v3) Create ExecMgr interface and single implementation: BaseExecMgr

I think house style is to use parameter names even on signatures/prototypes/declarations/etc, so Verifier::Verifier should have a parameter name for the new argument.

exec.h: "applicatble" and "Initiall" should probably be something else.

Comment 25

[Edwin Smith]

(In reply to comment #24)
> (From update of attachment 449676 [details] [diff] [review])
> I think house style is to use parameter names even on
> signatures/prototypes/declarations/etc, so Verifier::Verifier should have a
> parameter name for the new argument.
> 
> exec.h: "applicatble" and "Initiall" should probably be something else.

patches file is my own notes, meant to exclude from diff.  it wont be pushed.  will fix spelling and parameter naming.

Comment 26

[Edwin Smith]

(In reply to comment #11)

> So it's a bit of a hand-wavy suggestion, but I think the ExecMgr implementing
> class should define the available modes. Maybe RM_mixed can then be renamed to
> its truer semantics of 'RM_execmgr_default'.

agreed.  followon work captured in bug 570803.

Comment 27

[Edwin Smith]

http://hg.mozilla.org/tamarin-redux/rev/b64ec884b5c9