487199 - Support for multiple AvmCore instances on multiple threads within the same process

Status: VERIFIED FIXED

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

Description

[Lars T Hansen]

We need to make it possible to make a VM instance (the GC, any jitted and interpreted code, any data structures) from one thread to another in a process.  All the instances share the underlying GCHeap and FixedMalloc as per usual.

We know of at least two issues:

- the jit is slightly thread-dependent (Ed has a fix from Tinic)
- the GC and stack overflow checking must be made aware of the current 
  stack, this requires a new API

We also need some sort of testing setup for this.

Please log other issues and patches in this bug.

(Definition of done: we have a testing setup that verifies that this works.)

Comment 1

[Lars T Hansen]

Also see bug 487032.

Comment 2

[Lars T Hansen]

Also see bug 487322.

Comment 3

[Lars T Hansen]

Attachment: Shell refactoring v1

Major refactoring of the shell code as a preliminary step towards fixing this bug.

Shell has been split into two: "Shell", which has only static methods and exports the same static API as before (namely, "run()"), and which handles command line parsing, option checking, interaction for the repl, and so on; and "ShellCore", of which it should now (or soon) be possible to instantiate more than one.

This patch also fixes some bugs in command line parsing.

I belive I have inadvertently broken projector support in this patch and I'll look into that asap, but I'd like your opinion about the rest of it.

Comment 4

[Lars T Hansen]

Attachment: Additional fix for projector code

Mostly removing redundant code.  Not tested, actually.  Do we have tools for creating a projector file for the shell?

Comment 5

[Lars T Hansen]

Speaking of the projector, what are the conditions under which it may be run?  Here's what I can think of:

 - no file arguments presented
 - no -repl
 - no -Dselftest
 - the file is a projector file (duh)

The code in the first patch does not allow that but it's easy to fix, and in fact fixing it will further clean up the structure of the shell I think.

Comment 6

[Lars T Hansen]

Attachment: Shell refactoring v2

Merges the two previous patches and further refactors code.  Projector code still untested but has almost disappeared; projectors can now contain .abc, .swf, or even .as if the run-time compiler is present.

Comment 7

[Lars T Hansen]

Other concerns about thread support:

- VMPI calls that have non-atomic side effects, like VMPI_log
- "console", same situation

Comment 8

[Edwin Smith]

Comment on attachment 373642 [details] [diff] [review]
Shell refactoring v2

It'd be nice if arguments passed to a projector were available to AS3
as if they were given after -- with shell invocation.

* AVMFEATURE_SELFTEST defaults to 1, intentional?

* iirc, windows has an api for getting the exe pathname, and
i'm not sure if argv[0] is reliable.  but still a good starting
point for generic unixlike shells.

* avmshell.cpp: should AVM_SHELL_NO_PROJECTOR be used by itself
and let platform config headers weed out unsupported cases (UNDER_CE) ?

cleanup looks nice

Comment 9

[Lars T Hansen]

(In reply to comment #8)
> (From update of attachment 373642 [details] [diff] [review])
> It'd be nice if arguments passed to a projector were available to AS3
> as if they were given after -- with shell invocation.

They should be available now.  ShellCore::setup makes them available.

> * AVMFEATURE_SELFTEST defaults to 1, intentional?

It is; intended to disable before landing though.

> * iirc, windows has an api for getting the exe pathname, and
> i'm not sure if argv[0] is reliable.  but still a good starting
> point for generic unixlike shells.

Ok, good point.  Will take this into account when I test the projector.

> * avmshell.cpp: should AVM_SHELL_NO_PROJECTOR be used by itself
> and let platform config headers weed out unsupported cases (UNDER_CE) ?

It should, and it should not be called "_NO_", whatever else the case may be.  However, the issue here was that since this setting is for the shell only it's not part of the feature system.

It's possible that there are "conditional features" that are only present if AVMSHELL_BUILD is set, for example.  But should they really be exposed from the feature system?

Comment 10

[Edwin Smith]

(In reply to comment #9)

> It's possible that there are "conditional features" that are only present if
> AVMSHELL_BUILD is set, for example.  But should they really be exposed from the
> feature system?

hmm.  good question, i dont have an opinion yet. here i was just thinking honing in on the long ifdef and thinking along the lines of 

AVMFEATURE_PROJECTOR precludes <whatever-defines-UNDER_CE>

Comment 11

[Steven Johnson]

I didn't realize that avmshell had projector functionality, actually. Question: is it actually in use by anyone? Is it worth preserving?

Comment 12

[Lars T Hansen]

(In reply to comment #11)
> I didn't realize that avmshell had projector functionality, actually. Question:
> is it actually in use by anyone? Is it worth preserving?

We don't have tools to create projectors (yet), though trivial it may be.

Edwin's memory was that the projector only ever worked on Windows, yet as you see above he casts doubt on whether Windows actually exposes the executable name as argv[0], which is what the code has been using.

My guess is that the projector code hasn't been tested for a long time.

I think it may be worth preserving because it's pretty neat to be able to just ship a program as a blob and not have to ship multiple files - though the days when programs were files and not directory trees are probably coming to an end.  And if we support .swf we might as well try to support projectors?

Comment 13

[Lars T Hansen]

Comment on attachment 373642 [details] [diff] [review]
Shell refactoring v2

redux changeset:   1773:e8c43674e289

Comment 14

[Steven Johnson]

(In reply to comment #12)
> My guess is that the projector code hasn't been tested for a long time.

Likewise. My vote is that we lump this in with JavaGlue -- i.e., remove it for now rather than bother fixing it, with the plan on resurrecting it in the future when/if necessary. IMHO we have much more useful stuff to do, and any time spent maintaining projector code is probably a waste unless someone is relying on it.

Comment 15

[Lars T Hansen]

Attachment: Factor defaults for AvmCore so that they can be used to initialize shell settings structure

Fixes a regression

Comment 16

[Lars T Hansen]

Comment on attachment 373888 [details] [diff] [review]
Factor defaults for AvmCore so that they can be used to initialize shell settings structure

redux changeset:   1776:2ddf1afef17b

Comment 17

[Lars T Hansen]

Attachment: Multiworker solution (pthreads only)

Preliminary patch, this has been tested lightly on Mac and found to work.  Pthreads only, I'm in the process of porting to Win32 as well, and more testing needs to be done.

Comment 18

[Lars T Hansen]

Attachment: Multiworker solution (pthreads only), v2

Refactors the shell code into a command line handler / driver, and a portable ShellCore that takes care of many nitty-gritty details, like setting the stack height.

I've decided against trying to support Win32 threads in the shell driver (which is the only place it's needed at the moment), since Win32 threads before Vista are pretty far from pthread-compatible.  There are open-source pthread libraries we can use if we need to run the multiworker shell on Win32 in the near future.

This patch is reasonably clean but not quite ready to land, remaining work is:

- test with more substantial apps
- test on non-Mac pthreads platform
- deal with stack height - query the pthreads implementation about the actual stack height, and/or request a specific stack height
- disable the debug logging that's currently in the code
- incorporate ShellCore in the vcproj and manifest.mk

Comment 19

[Lars T Hansen]

(In reply to comment #14)
> (In reply to comment #12)
> > My guess is that the projector code hasn't been tested for a long time.
> 
> Likewise. My vote is that we lump this in with JavaGlue -- i.e., remove it for
> now rather than bother fixing it, with the plan on resurrecting it in the
> future when/if necessary. IMHO we have much more useful stuff to do, and any
> time spent maintaining projector code is probably a waste unless someone is
> relying on it.

Removal of those two aspects logged as bug #489986.

Comment 20

[Lars T Hansen]

Attachment: Multiworker solution (pthreads only), v3

This patch is missing changes to the visual studio project files; I will incorporate those before landing.

This patch requires the patch to bug #456054 (stack limit computation) and is otherwise based on redux changeset 1789:c064e34758e0.

I've tested this by running the scimark tests repeatedly on multiple cores on multiple threads, using the switch -workers 3,2,3 (for example), and everything appears to be solid.

(Regarding porting the shell driver code to non-pthreads, that would be nice but is IMO out of scope for this fix.)

Comment 21

[Lars T Hansen]

Remaining issues:

- Is the threadlocal variable abstraction used in mmgc resilient against
  threads coming and going?  (Consider a tlv being created while the vm is
  on thread A; later, A exits, after which the same vm runs on some thread B.)

- Numerous uses of static variables (globals and on the function level) are
  currently not protected by critical sections, and almost certainly need to
  be to be portable.

Comment 22

[Lars T Hansen]

Comment on attachment 374656 [details] [diff] [review]
Multiworker solution (pthreads only), v3

redux changeset:   1794:6b74f450af2a

Comment 23

[Lars T Hansen]

Renaming to capture the two new items in comment #21, plus the following:

- Is each GC + AvmCore pair using little enough memory?  I'll attach results from a study.

- Is the startup and shutdown time of a new GC + AvmCore pair adequate?

Comment 24

[Lars T Hansen]

Also want to expose GC::DisableThreadCheck somehow (see bug #487322).  Probably call it when configuring a fresh ShellCore.

Comment 25

[Tommy Reilly]

When GetStackTop goes away and MMGC_GCENTER becomes mandatory we don't need to ever disable the thread check and that ability goes away I believe.

Comment 26

[Lars T Hansen]

(In reply to comment #21)
> 
> - Is the threadlocal variable abstraction used in mmgc resilient against
>   threads coming and going?  (Consider a tlv being created while the vm is
>   on thread A; later, A exits, after which the same vm runs on some thread B.)

It is.  I've checked Windows, pthreads, and Symbian, and the common theme is a level of indirection where a global key is constructed for a datum and used to access a thread-local database to get/set the datum.  Ergo we seem to be insulated from the thread identity.

Comment 27

[Lars T Hansen]

(In reply to comment #26)
> (In reply to comment #21)
> > 
> > - Is the threadlocal variable abstraction used in mmgc resilient against
> >   threads coming and going?  (Consider a tlv being created while the vm is
> >   on thread A; later, A exits, after which the same vm runs on some thread
> >   B.)
> 
> It is.  I've checked Windows, pthreads, and Symbian, and the common theme is a
> level of indirection where a global key is constructed for a datum and used to
> access a thread-local database to get/set the datum.  Ergo we seem to be
> insulated from the thread identity.

Actually this may not quite be the case.  The design of TLS is resilient, but there may be uses of TLS that are not.  The Sampler stores a pointer to the AvmCore it's created for in a TLS when the Sampler is created, and later Sampler functions access the core from that TLS.  But if a core moves from thread to thread (and the sampler does too) then the sampler will read the TLS of some thread it was not created on, which may be another core (confusion ensues?) or possibly NULL.

The uses of TLS in the VM are:

- memtag and memtype in the profiler (these are global vars)
- enterframe in GCHeap (a member variable); since GCHeap is a singleton
  there's only one
- tls_sampler for the Sampler (this is a global var)

Of these, only tls_sampler is problematic in the way outlined above.

Comment 28

[Lars T Hansen]

Attachment: Comments for cases we won't fix - debugging code

First set of patches for non-thread-safe global variables.  These cases are all white-box debugging functionality: vprof and so on.  The "fixes" just comment the enabling statements for the functionality and the comments note that multiple live avmcores are not supported.

Comment 29

[Lars T Hansen]

Attachment: Initializing caches before any cores are created

This patch makes sure that some statics/globals that are used as lazy-init caches are initialized before the first AvmCore is created.  This is invariably done by having a global/static initializer that calls the function in question so that it initializes its cache.  That is not portable but these cases are all in porting layers (the one in GC.cpp is inside an #ifdef SOLARIS).

Comment 30

[Lars T Hansen]

Attachment: Demote variables that don't need to be global

Demote one variable that was writable and static but never written and never used except locally, to a local const, adding a comment about what it would take to go the other way.

Comment 31

[Lars T Hansen]

Attachment: Refactor the windows spinlock so that it can be used by the platform layer without the VMPI rigamarole

Comment 32

[Lars T Hansen]

Attachment: Add locking to those remaining globals that need it

We use the refactored windows spinlock where possible (does not create problems with deallocating the lock objects at shutdown) and the last lock is hung off of GCHeap where it logically belongs (more or less) and is cleaned up when GCHeap is destroyed.

Comment 33

[Lars T Hansen]

(In reply to comment #23)

> - Is each GC + AvmCore pair using little enough memory?  I'll attach results
> from a study.

Here are the data from the study, recorded for posterity.

There are two programs, 'nop' and 'massive' (which I will attach).

'nop' performs '1+2' and exits (doesn't even print it).

'massive' instantiates every ES3 built-in type and calls each method on it, all from within one function.

We run with -workers 100,1,200, which means 100 cores, one thread, and 200 repititions - in other words, run the program twice on each core in round-robin fashion.

The programs are compiled with -optimize.

We measure the peak of private memory as reported by -memstats.

Nop:
  With -Ojit (and wordcode) we reach 71.5 MB
  With -Ojit (and abc) we reach 62.3 MB
  With -Dinterp wordcode we reach 57.5 MB
  With -Dinterp abc we reach 47.2 MB

Massive:
  With -Ojit (and wordcode) we reach 103.5 MB
  With -Ojit (and abc) we reach 99.9 MB
  With -Dinterp wordcode we reach 72.3 MB
  With -Dinterp abc we reach 55.5 MB

We observe a fairly high cost per instance (it takes 470KB per instance to play if you're willing to run the interpreter and basically not do anything; 1MB if you want to use the JIT and run a program that does something).

We observe that wordcode adds significantly to memory consumption, and while this has not been investigated deeper it supports our assumption that the three viable configurations of the VM are ABC alone; wordcode alone; and ABC+JIT.

Not shown in the numbers above is the number of garbage collections during this exercise, which seems absurdly high: 646 measured for -Ojit nop.  (That should be taken as preliminary.)

There also appears to be a large number of GCs during shutdown, it looks like the GC is tweaked eagerly by core deletion?  This too needs to be examined in more depth.

Comment 34

[Lars T Hansen]

Attachment: The program 'nop'

Comment 35

[Lars T Hansen]

Attachment: The program 'massive'

Comment 36

[Edwin Smith]

Comment on attachment 376215 [details] [diff] [review]
Initializing caches before any cores are created

kudos for cleverness, this makes the linker put the init code early enough before threads are spawned.  

neat.  is it portable?

regarding the previous patch about initializing the interpreter direct-threading labels "very early" -- should this patch do that too?

Comment 37

[Lars T Hansen]

(In reply to comment #36)
> (From update of attachment 376215 [details] [diff] [review])
> kudos for cleverness, this makes the linker put the init code early enough
> before threads are spawned.  
> 
> neat.  is it portable?

It isn't, unfortunately - not all platforms can deal with static initializers.  So these fixes are all in platform code where I know it works (Mac, Win, Solaris).

> regarding the previous patch about initializing the interpreter
> direct-threading labels "very early" -- should this patch do that too?

Modulo portability issues, but in the case of MSVC (the only case right now) it should work cross-platform.  I'll note that in a comment before I land it.

Comment 38

[Lars T Hansen]

Somebody inquired about 64-bit, so here are the numbers:

Nop:
  with -Dinterp (abc code): 74.1MB
  with -Ojit (abc code): 75.9MB

Massive:
  with -Dinterp (abc code): 90.1MB
  with -Ojit (abc code): 79.2MB

The massive/interp number makes scant sense but it's reproducible.

Comment 39

[zwetan]

> 
> * iirc, windows has an api for getting the exe pathname, and
> i'm not sure if argv[0] is reliable.  but still a good starting
> point for generic unixlike shells.
> 

I patched a bit the avmshell to support argv[0] (badly needed it for projector)

see
http://code.google.com/p/redtamarin/source/browse/trunk/src/shell/SystemClass.cpp#169
http://code.google.com/p/redtamarin/source/browse/trunk/src/shell/SystemClass.cpp#189
http://code.google.com/p/redtamarin/source/browse/trunk/src/shell/shell_toplevel.as#81
http://code.google.com/p/redtamarin/source/browse/trunk/src/shell/avmshell.cpp#498
http://code.google.com/p/redtamarin/source/browse/trunk/src/shell/avmshell.cpp#896

under OSX/Linux I simply used the argv[0] or reuse the 'executablePath' var
under Windows I used GetModuleFileName (Mmsystem.h)

I didn't extensively tested it but it's working pretty consistant so far

Comment 40

[Lars T Hansen]

(In reply to comment #39)
> 
> I patched a bit the avmshell to support argv[0] (badly needed it 
> for projector)

If you wouldn't mind terribly generating a conventional patch for this and attaching it to this bug, I would be happy to take ownership of it and sheperd it into the code.

Comment 41

[Lars T Hansen]

Comment on attachment 376214 [details] [diff] [review]
Comments for cases we won't fix - debugging code

redux changeset deccebc6f2a3

Comment 42

[Lars T Hansen]

Comment on attachment 376215 [details] [diff] [review]
Initializing caches before any cores are created

redux changeset deccebc6f2a3

Comment 43

[Lars T Hansen]

Comment on attachment 376216 [details] [diff] [review]
Demote variables that don't need to be global

redux changeset deccebc6f2a3

Comment 44

[Lars T Hansen]

Comment on attachment 376220 [details] [diff] [review]
Refactor the windows spinlock so that it can be used by the platform layer without the VMPI rigamarole

redux changeset deccebc6f2a3

Comment 45

[Lars T Hansen]

Comment on attachment 376221 [details] [diff] [review]
Add locking to those remaining globals that need it

redux changeset deccebc6f2a3

Comment 46

[Brent Baker]

changeset 1877:deccebc6f2a3 does not compile on solaris:

MMgc/GC.cpp", line 1779: Error: The function "validaddr" must have a prototype.

Comment 47

[Lars T Hansen]

Attachment: Compile fix for solaris

Not tested, but plausible.

Comment 48

[Brent Baker]

Comment on attachment 377146 [details] [diff] [review]
Compile fix for solaris

confirmed that solaris compiles again with this patch

Comment 49

[Lars T Hansen]

Comment on attachment 377146 [details] [diff] [review]
Compile fix for solaris

redux changeset a95b20ef2590

Comment 50

[Lars T Hansen]

(In reply to comment #40)
> (In reply to comment #39)
> > 
> > I patched a bit the avmshell to support argv[0] (badly needed it 
> > for projector)
> 
> If you wouldn't mind terribly generating a conventional patch for this and
> attaching it to this bug, I would be happy to take ownership of it and sheperd
> it into the code.

Moved to bug #497067.

Comment 51

[Lars T Hansen]

No known issues remaining.