419872 - Hosting infrastructure

Status: RESOLVED FIXED

(Tamarin Graveyard :: Tracing Virtual Machine, defect)

Description

[Tommy Reilly]

In TC host's called into the VM using a handful of entry points:

ScriptObject::call
MethodEnv::coerceEnter
Toplevel::constructObject

We want to eradicate this practice and have one mechanism for calling in.  Further more these entry points will be restriced to static method that only take primitives as arguments.  The idea is all the VM state be only stored in the VM.  This may be overly restrictive but we'd rather be overly closed and loosen up than start wide open and get stuck there (current situation).   

So a entry point is declared in AS and GlobalOptimizer generates the necessary glue code to call into the interpreter.

Comment 1

[Tommy Reilly]

Attachment: prototype

This shows the glue hand coded at the top of avmshell.cpp, this is what G.O. will generate.  Mostly looking an okay that everything interpMethodEnv is doing is okay, is okay to pull the MethodEnv out of the AbcEnv like that or should I go through the TraitsEnv?  This seemed the most straightforward approach.

Note that this doesn't work, still working out the kicks with how I'm passing the args to forth.

Comment 2

[Steven Johnson]

pulling  the ME our of AbcEnv is ok in this case I think.

You're restarting the sp at stack, won't it step on existing stack items? ditto for rp.

Are the Atom args to avmplus_System_private_callin just to allow for null and undefined? if so we should come up with another way so we don't have to expose Atom if possible.

nit: argc should be uint, not int

probably good to have Edwin look at this too

Comment 3

[Tommy Reilly]

I'm not concerning myself with re-entrancy at the moment, basically its assumed anything using this callin approach is a root entry point, I suspect we'll get a lot of mileage out of that simplification (ie I think we can get pretty far w/o re-entrancy support).

Hmm, yeah supporting Atom probably isn't necessary, I'll remove it.  Where I'm struggling is with the stack setup, I'm replacing w_callenv, with w_callin and that will be a stripped down version of callenv, basically I think I can avoid all the coercion and stack prep stuff by getting the stack right in interpMethodEnv.  Let me know what you think of that, I haven't gotten my head around how the forth stack setup needs to work.  So far I have

EXTERN: w_callin ( obj args argc env -- result )
	PUSHFRAME          ( obj args argc env )
	methodinfopc SETPC	( obj args argc )
	ENV getforthimpl	( obj args argc wimpl )
	EXECUTE				( result )
	POPFRAME ;

Comment 4

[Steven Johnson]

yeah, for a C->ABC call it should be fine to require the stack correct ahead of time, I think it should be possible to avoid the coercion. though the coercion probably won't make much difference.

re: forth stack setup, yeah, it's messy. you may need to wrap the execute in enterabc/exitabc (or is that encapsulated inside the called code? not sure)

re: re-entrancy, got it, that would make life much simpler. (probably be good to add some assertions checking for that.)

Comment 5

[Tommy Reilly]

Attachment: working prototype

This is working, now I'm off to add the stub generation to G.O. everything seem kosher?

Comment 6

[Tommy Reilly]

Attachment: GlobalOptimizer stub generation diff

This is the asc part.

Comment 7

[Tommy Reilly]

Example:

class Foo
{
   static private function callfromC(arg:int):bool
   { 
      trace("abc, easy as " + arg);
      return true;
   }
}

From C:

bool b = avmplus_Foo_private_callfromC(123);

The compiler generates avmplus_Foo_private_callfromC in the builtin cpp file and a decl in the .h file.

Comment 8

[Tommy Reilly]

Oops there supposed to be a [native] metadata tag on the function

Comment 9

[Mark Hammond [:markh] [:mhammond]]

This sounds very useful.  However, it only supports a "static" model and some facility must be provided for a more dynamic model (a-la ScriptObject::call and the like) - for example, when the name of the function to call is provided at runtime.  Assuming we had catchalls implemented, I guess this would still work - we can pass the name of the method-to-call/property-to-set-get to script code, which implements the dynamic model.  But IIUC, catchalls aren't close.  I expect I'm reading too much into ScriptObject::call being listed above though...

Comment 10

[Edwin Smith]

Comment on attachment 306318 [details] [diff] [review]
working prototype

I would prefer to see this require a toplevel function as the entry point rather than a static class method.  for one thing, it avoids the overhead of a class when its not needed.  it also eliminates the need for class_id, just need method_id.

i will pull the patch and see what the changes would look like, before i push to hard on the idea.

Comment 11

[Edwin Smith]

(In reply to comment #9)
> This sounds very useful.  However, it only supports a "static" model and some
> facility must be provided for a more dynamic model (a-la ScriptObject::call and
> the like) - for example, when the name of the function to call is provided at
> runtime.  Assuming we had catchalls implemented, I guess this would still work
> - we can pass the name of the method-to-call/property-to-set-get to script
> code, which implements the dynamic model.  But IIUC, catchalls aren't close.  I
> expect I'm reading too much into ScriptObject::call being listed above
> though...
> 

The idea is to keep the C->ABC transition as minimal as possible, any other niceities such as a dynamic invocation point, a class, complex parameters, and/or exception handling, can be done in AS3.

I would even go so far as to say if we think we need a "standard" embedder-friendly api, with all the bells and whistles, that even that API should be done in AS3.

Comment 12

[Tommy Reilly]

(In reply to comment #10)
> (From update of attachment 306318 [details] [diff] [review])
> I would prefer to see this require a toplevel function as the entry point
> rather than a static class method.  for one thing, it avoids the overhead of a
> class when its not needed.  it also eliminates the need for class_id, just need
> method_id.
> 
> i will pull the patch and see what the changes would look like, before i push
> to hard on the idea.

I wasn't sure how to get the right global object to be the reciever for that case.  Also I thought the ability to declare them in a class was a better fit for the player, maybe we could support both?

Comment 13

[Edwin Smith]

The class objects are sitting in the global object, so whatever youre doing now is going through the desired global object, to get the class object.  and, initializing it as a side effect, which is an implicit callin.  so, one less load should do it.  Also youre using a method_id for the method to call, just as easy to get the method_id for the global function.  code would look like this:

package
{
  [native-entry-powerjam]
  function entrypoint() {}
}

I'm playing with this as we speak, let me just see what the code looks like.  I dont think this needs to be user friendly at all, all the user-friendly abstractions you want could be done in as3.

my desire is to only support global methods but supporting both, or just class-static methods, could be a compromise if there is some major pain point with global functions.

Comment 14

[Edwin Smith]

Comment on attachment 306318 [details] [diff] [review]
working prototype

for the hostClass array on toplevel to work right, all the classes that have entry points have to be in the same abc file.  otherwise class_id's will recycle and collide.

Comment 15

[Edwin Smith]

Comment on attachment 306318 [details] [diff] [review]
working prototype

scalpel in the patient?  

-		const bool t = allowTracing ? tracing() : true;
+		const bool t = true;//allowTracing ? tracing() : true;

You're stowing away the CC object in newclass(), which means someone else had to call some other entry point to initialize the global object first.  

the proof of concept would be to wrap the code in shell/main.as in a function (or class) -- make sure we can bootstrap the vm with an entry point callin

Comment 16

[Edwin Smith]

Comment on attachment 306361 [details] [diff] [review]
GlobalOptimizer stub generation diff

code looks fine, but if you can macro-ize any of the boilerplate that goes into a callin method, it'll save you a lot of headache later.  i.e. the output files (.cpp, .h) use macros where possible, for boilerplate code, so mostly if the code has to change, the macros are changed, instead of changing G.O.

Comment 17

[Tommy Reilly]

yeah I was thinking at some point we may want to optimize it further so that the args are written to the real stack but as long as the mechanism is fixed the generated code isn't really important as its all hidden from the host.  But yeah, point taken, keeping the native knowledge in the asc to a minimum would be good, in particular I don't like how the G.O. knows so much about Boxes.

Comment 18

[Steven Johnson]

Hey Tommy, one thing we should probably work on while we're at it is providing a well-defined interface for native methods; right now they need to include internal headers and know about the guts of things like ScriptObject and Box, plus, they might need to be able to call getprop or getuintprop on an array argument passed to them... we should really create a "public" api that can insulate TT clients from the internals of TT.

Comment 19

[Tommy Reilly]

yeah Ed and I discussed this, the idea is that the vm will generate an init method and vm startup looks like (for the shell):

avm vm = shell_init(); // init pools, create DominEnv, Toplevel etc
avmplus_main(vm);

both of these methods are generated by G.O.  "vm" is an opaque pointer to an AbcEnv.  ScriptObjectp would need to be exposed for callout methods self and classself args but it can probably just be declared and not defined (ie another opaque pointer).

Once I get all this baked I'll have a go at creating a "embedding" API.  I'm thinking avm.h (I love short names!)

Current list:

space.h
avmplusTypes.h
String.h
NativeFunction.h
Debugger stuff

There's a couple things in the way of making this work:

- ConsoleOutputStream crap, just goes away , avmshell wires System._write to stdout
- interupt handling
- stack overflow handling

Another idea that came up was to merge the builtin abcs so there's only one (probably a couple hundred bytes of savings there)

Comment 20

[Edwin Smith]

I would love to see Console/PrintStream gone from the vm code, if anything maybe its embedder can provide a FILE*.  reeks of commandline, but even for fancy debug output routing how hard could it be?

what really needs to be done for stack overflow handling?  the vm throws a stack overflow exception, which can be caught in a userdefined callin.  

Comment 21

[Tommy Reilly]

okay, so far so good, what about CodeContext, shouldn't DomainEnv* suffice for its purposes?  The player subclasses it to create a ScriptPlayer*/SecurityContext*/DomainEnv* tuple.  It appears to need to get from the executing code to the swf (ScriptPlayer*).  

Comment 22

[Benjamin Smedberg]

I have a few concerns that may or may not be valid:

1) can we avoid having the global function pollute the global namespace? Or, the namespace for the global function avoid polluting the set of namespaces? Or, are there namespaces which are reserved for ES4 compilers?

2) Is declaring the private function "private" enough to prevent script callers from calling it? I know we haven't gotten into discussions about security models yet, but I don't want to design in a system which is hard to secure later.

3) I don't understand the comments above about re-entrancy. We're definitely going to need re-entrancy for even simple operations such as a DOM modification firing event listener notifications.

4) assuming that we'll have a few standard callback functions: what would an efficient way to call them with variable number of args? Creating an array for a varags function sounds like a lot of overhead.

Comment 23

[Tommy Reilly]

Responding to (In reply to comment #22)

> 1) can we avoid having the global function pollute the global namespace? Or,
> the namespace for the global function avoid polluting the set of namespaces?
> Or, are there namespaces which are reserved for ES4 compilers?

Each .as file gets its own "global" and only public symbols are exported to the rest of the domain, so if you use private then the symbol isn't visible outside the .as file.
 
> 2) Is declaring the private function "private" enough to prevent script callers
> from calling it? I know we haven't gotten into discussions about security
> models yet, but I don't want to design in a system which is hard to secure
> later.

Only script in the same .as file could call it.

> 3) I don't understand the comments above about re-entrancy. We're definitely
> going to need re-entrancy for even simple operations such as a DOM modification
> firing event listener notifications.

Re-entrancy is on the table, no worries.  

> 4) assuming that we'll have a few standard callback functions: what would an
> efficient way to call them with variable number of args? Creating an array for
> a varags function sounds like a lot of overhead.

We haven't thought about variadic callins, how important are they?  We haven't come across a need for them yet although we probably will...

Comment 24

[Tommy Reilly]

If we do add variadic call in the interface will probably be (int argc, Box* args) so I wouldn't worry about Array overhead.

Comment 25

[Benjamin Smedberg]

The Box* solution is perfect. But yeah, we are going to be doing lots of variadic calls, because we don't know the signatures of IDL methods at compile-time, so we'll be constructing the correct call signatures at runtime.

Comment 26

[Tommy Reilly]

Attachment: Take2

Getting closer, highlights:

1) AvmCore subclassing gone, "Shell" class gone and Configuration object introduced
2) vm bootstrap all encompassed in generated shell_init function. 
3) CodeContext/ShellCodeContext gone, thinking is AbcEnv will suffice
4) ConsoleOutputStream moved into core, this probably needs more thinking, like can we have ConsoleOutputStream route output back through System.write so the host can handle it?
5) DomainClass.cpp obviously needs to move into core, not sure how that should look, will take a stab at that next.
6) I cleaned up avmshell.cpp pretty heavy-handidly, I suspect most of this was scapels in the patient.
7) added shell_destroy method because the AvmCore needs to be destroyed.  Thinking about make AvmCore a GC object to make this unnecessary.

Comment 27

[Edwin Smith]

(In reply to comment #22)

> 1) can we avoid having the global function pollute the global namespace? Or,
> the namespace for the global function avoid polluting the set of namespaces?
> Or, are there namespaces which are reserved for ES4 compilers?
> 
> 2) Is declaring the private function "private" enough to prevent script callers
> from calling it? I know we haven't gotten into discussions about security
> models yet, but I don't want to design in a system which is hard to secure
> later.

Adding to Tommy's reply.  With the latest drop, entry points are toplevel functions instead of static methods on classes.  They are in a user defined package, but aren't public.  This makes them "internal" (like java package-private), but not completely private.  AS3 currently lacks syntax to define fully private toplevel names.

all is not lost.  the VM supports fully private toplevel names.  The packager (GlobalOptimizer) that creates the builtin abc's and .cpp and .h files, turns these internal namespaces into private namespaces.   the net effect is like:

package mystuff {
   [native]
    private function main()
}

I would be interested to see if we can go further, and allow this:

package private {
}

but either way you get no name pollution, and referential security.  private names can only be referenced in the same abc where they are defined.  the VM compares them by pointer, whereas non-private namespaces are compared by value (uri).  

Comment 28

[Edwin Smith]

echoing Ben Garney, we should rename [native] to [callin] or [entrypoint] or something, since native is already an attribute that means something else.

nevermind about package private {}, its more useful to put entrypoints into packages along with classes they use, and have the packager (GlobalOptimizer) turn |internal| into |private| where necessary.

Comment 29

[Tommy Reilly]

Attachment: take 2 for asc changes

Corresponds to Take2 patch, adds scriptId stuff, _init generation etc.  Needs cleaning up.

Comment 30

[Edwin Smith]

* (nit) would be nice for the loop: while((bindingCursor =  to be in a helper
method.  is it just to catch compiler bugs?

* the if (! ... everCalled()) check roughly duplicates the |initglobal| forth
word  (which it calls -- so we're basically doing if (!everCalled()) { if
(!everCalled()) { /* init */ }}, if you look at the code flow.

could we refactor w_callin so it takes care of that?   wouldn't even have to
change w_callin's signature, since obj is the global object that everCalled is
checking.    something like:

: w_callin ( obj args argc env -- result )
    OVER 2+ PICK initglobal DROP 
    // current body of w_callin

this means you don't have to call into the interpreter, exit, then call back
in.  you just call w_callin and it takes care of the rest.

* in interpMethodEnv(), it looks like youre pushing the global obj an extra
time, after the args but before the argc. and, pushing the args in reverse
order. i dont see why

* making AvmCore a gc object sounds fine, but is there still usefulness in a vm
shutdown hook, eg for printing stats?  if were going to make it optional and
leave it up to the embedder, maybe it belongs in AS3? i'm ambivalent for the
moment.  

Comment 31

[Tommy Reilly]

Attachment: Latest asc patch, abc merging!

Comment 32

[Tommy Reilly]

Comment on attachment 307503 [details] [diff] [review]
Take2

Cancelling review of this, will ask for review of final patch once the trace tree stuff lands and I rebase to tip).

Comment 33

[Tommy Reilly]

Attachment: ASC callin patch, abc merging, callin generation

Comment 34

[Tommy Reilly]

Attachment: Callin/hosting stuff, should be ready for push (win/linux/mac tested)

Its noisy due to the uint8_t changes

Comment 35

[Tommy Reilly]

This patch needs more work.  It runs the regression tests and the sunspider benchmarks fine but fails on esc compiling debug.es which TT tip handles fine.   Hmm, esc might be the next frontier in regression tests...

Comment 36

[Tommy Reilly]

Attachment: Hosting changes and more

Comment 37

[Steven Johnson]

Comment on attachment 313107 [details] [diff] [review]
Hosting changes and more

Looks ok -- can we get documentation (even a rough wiki page) on the new callin/hosting setup soon, though?

Also, in the future maybe we should do the giant type substitutions (byte->uint8_t) as separate patches :-)

Comment 38

[Edwin Smith]

Comment on attachment 310683 [details] [diff] [review]
ASC callin patch, abc merging, callin generation

looks good, but what does this review really mean?  i would assume there is a corresponding review process for asc changes using a JIRA bug and patch before pushing to svn?

Comment 39

[Tommy Reilly]

I talked to Jono and he said there is no such process so I was gonna just piggy back on our bugzilla process here.  I plan on quoting this bug in my svn submission.  Agreed though for core asc changes a JIRA bug would be better, but since the community of people who even know what GO is is ~3 people I thought it overkill.   

I thought you might have reservations about some of the tweaks I did to try to get GO to do all the verifier poking optimizations that I eventually bailed on, they all look kosher?

Comment 40

[Edwin Smith]

(In reply to comment #39)

> I thought you might have reservations about some of the tweaks I did to try to
> get GO to do all the verifier poking optimizations that I eventually bailed on,
> they all look kosher?

They looked pretty benign, as long as all the regressions & sundry pass okay then I don't have any reservations.

Comment 41

[Tommy Reilly]

asc changes pushed to svn as revision 1089

Comment 42

[Tommy Reilly]

tt changes pushed as revision 331