591492 - Code built on top of VMPI that's shared between MMgc/VM/player should have a separate dir

Status: RESOLVED FIXED

(Tamarin Graveyard :: Build Config, enhancement)

Description

[Simon Wilkinson]

Currently, platform independent code built on top of VMPI that is shared between the MMgc/VM/player has no place to live apart from in the VMPI directory or MMgc.

For enhancements such as a shared threading library (bug 555765), neither location is attractive. A new directory should be created for this layer.

See Lars' comment https://bugzilla.mozilla.org/show_bug.cgi?id=555765#c13

Comment 1

[Simon Wilkinson]

Suggestions for this namepace/layer/module/dir's name:

Very generic:
- common
- base
- shared

Or initially it could be specific to threads/VM-concurrency:

- threads
- concurrent
- concurrency_utils
- sync

We can also mix these and have a relevant prefix / postfix too (i.e. avm_ or _avm)

My initial suggestion:

- Have a namespace 'base', which is to contain classes which do not have a home in avmplus or MMgc.
- Place the new concurrency classes in the base::concurrency_utils namespace.
- Place files in a new directory called 'base'.

Comment 2

[Lars T Hansen]

I'm not sure what the nested namespaces do for us, can you elaborate?  In practice it's common to open the namespaces you need where you need them anyway, so namespaces in Tamarin are for logical grouping more than partitioning the namespace per se.

How about a single directory 'base' and everything in that namespace being in a namespace 'avmbase'?  (This namespace will in practice be open in every file in Tamarin anyway since eg AvmAssert will be in it.)

Comment 3

[Simon Wilkinson]

(In reply to comment #2)
> I'm not sure what the nested namespaces do for us, can you elaborate?  In
> practice it's common to open the namespaces you need where you need them
> anyway, so namespaces in Tamarin are for logical grouping more than
> partitioning the namespace per se.

I was thinking about the case where the FP wants to use these classes. E.g. if the new concurrency classes replace TSafeThread, CriticalSection etc., then having a specific namespace looked much cleaner. I presume there'll be other things in the base namespace which will have no relevance for the FP.


> How about a single directory 'base' and everything in that namespace being in a
> namespace 'avmbase'?  (This namespace will in practice be open in every file in
> Tamarin anyway since eg AvmAssert will be in it.)

Again, does this look right from the FP's point-of-view?
Or should we not care?

Comment 4

[Simon Wilkinson]

Lars,

Can you provide some other examples of classes/files that needing moving into this new layer?

You mention AvmAssert in bug 555765#c13.

I'd like to prepare a patch and land this asap...

Comment 5

[Edwin Smith]

Tamarin doesn't have a formal rule for what a module is, but loosly speaking, a module is a hunk of functionality we'd want to manage separately - it has its own dir, its on public api + include file, several private api's and impl files.  if we used DLL's, then module = dll.  In principle, its a hunk of functionality we might even dynamically link and version separately.  

Examples of our exsiting quasi-modules are:
eval  - but, very tiny api
nanojit - but, private api's leak into public nanojit.h  (nanojit even has its own repository, and any module potentially could have its own)
mmgc - but, no public/private api split, and messy dependencies between the vm and mmgc.
vmpi
shell - even has its own namespace, but no api.  and, has as3 api's that belong elsewhere.  this quasi-module should either be the commandline wrapper for a lib, or a lib itself, but not both.

some non-modules directories that are probably anti-patterns:
core - just a dumping ground, no structure
extensions - mix of extra as3 api's, and self tests

We have no desire to get too fine grained with modules.  an assert module all by itself is silly.  other code that has similar characteristics includes the container classes (List<>, MultinameHashtable<>, Hashtable<>), and probably the I/O wrappers (PrintWriter, ConsoleOutpuStream, etc).  the edges are blurry.

So, we definitely need a place for the not-mmgc and not-vm code, like assert, list<>, and so on, and we also need a place for threading.

the only remaining issue is whether we think the threading code is meaty and distinct enough to be in its own module, vs being lumped in with the other stuff.

if its in its own module, then there should be a toplevel dir, a public include file (exposing as little as possible, but as much as necessary since C++ is kind of sucky in this way).

(naming - "threads" or "concurrent" are cool with me, as would).  i think nesting namespaces + underscores are overdoing it.

if we are worried about C++ namespace collision, i'd suggest 'vm' as the prefix, with no underscores.  (if we had it to do over, the mmgc namespace might better be vmgc or avmgc).  VMPI got its name that way.  (avmthreads rolls of the tongue, but the 'a' adds nothing.  so, vmthreads?  

in summary, how about:  "vmbase", or "vmbase + vmthreads".  the new threading code is meaty enough to justify being its own module in the long run.  is it?

Comment 6

[Simon Wilkinson]

(In reply to comment #5)
> in summary, how about:  "vmbase", or "vmbase + vmthreads".  the new threading
> code is meaty enough to justify being its own module in the long run.  is it?


I do not think the new threading code deserves it's own module due to its size or complexity.
The only reason would be to allow separate linking with the FP, but I do not know if this is a scenario we need to consider. If not, then 'vmbase' sounds good to me. As the module name, the directory name, and the namespace?

Comment 7

[Simon Wilkinson]

Attachment: AvmAssert moved into vmbase module (Inc. command-line build changes)

This patch defines a new toplevel directory, 'vmbase', which is to contain the new vmbase module.

As an example, and to test the build, AvmAssert has been moved into the vmbase directory, and within the vmbase namespace.
Note that avmplus.h and MMgc.h now #include vmbase.h rather than AvmAssert.h, and that the vmbase namespace is opened in avmplus.h

The command-line build system has been updated to build and link the vmbase library.

Comment 8

[Simon Wilkinson]

Attachment: Updated Xcode project

Comment 9

[Simon Wilkinson]

Attachment: Updated VisualStudio project

Comment 10

[Simon Wilkinson]

Attachment: Updated Eclipse project

Why is this so large?

Comment 11

[Lars T Hansen]

Comment on attachment 473300 [details] [diff] [review]
AvmAssert moved into vmbase module (Inc. command-line build changes)

I'm not fond of "vmbase" when we already have "avmplus" and we're not likely to get rid of the latter easily; "avmbase" or probably even "avmplusbase" would have been better IMO (with "avmplusgc" and "avmpluseval" etc coming eventually).  On the other hand, as Ed points out we have "VMPI" and "VMCFG_" and -- apart from the names of the executables -- we've tried to move in that direction generally, so I suppose it makes some sort of sense and I'm not going to make more of a stink about it.  (The names of bound variables do matter, but they're not the only things that matter :-)

Comment 12

[Edwin Smith]

i'm fine with 'avm' or 'vm'; its odd as a directory name prefix, but not as an include file name prefix.

there's two possible schemes for how this pans out:

1. 'avm' everywhere, down with 'plus' and 'vm', and 'MM'.  maybe VMPI should have been AVMPI, etc.  module names are avmgc, avmcore, avmshell, avm.exe, avm.dll, etc.

2. 'avm' for anything actionscript specific, 'vm' for generic stuff.  so, 'vmgc', vmbase', avmcore, avmshell, and so on.  

From The Matrix, Morphius to Neo:  "You take a chance either way.  I leave it to you.  <click>"

Comment 13

[Lars T Hansen]

Let's try to move this along - I'm fine with vmbase, don't let me stand in the way.  We can worry about the others when we get to them.

Comment 14

[Edwin Smith]

Comment on attachment 473300 [details] [diff] [review]
AvmAssert moved into vmbase module (Inc. command-line build changes)

vmbase FTW!

Comment 15

[Lars T Hansen]

I'll push.

Comment 16

[Lars T Hansen]

Comment on attachment 473308 [details] [diff] [review]
Updated VisualStudio project

Appears to require some surgery, include paths may not have been updated everywhere it needs to be (notoriously laborious).

Comment 17

[Lars T Hansen]

tamarin-redux changeset:   5265:190fcd8bbb91

(Including some vcproj fixes for vs2008, but not including the eclipse patch - I've asked Simon for fresh bits.)

Comment 18

[Lars T Hansen]

BTW in bug #599277 we will discover that we'll need vmbase-inlines.h, and that it is to be included in both avmplus.h and MMgc.h at the start of the inline includes.

Comment 19

[Lars T Hansen]

Added search paths to Xcode project:
tamarin-redux changeset:   5274:d2d68739a882

Eclipse files (update from Simon):
tamarin-redux changeset:   5275:300d272944b4