440478 - Improve modularity and make nanojit and the tracer less dependent on the VM core.

Status: RESOLVED FIXED

(Tamarin Graveyard :: Tracing Virtual Machine, enhancement)

Description

[Andreas Gal :gal]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_5_3; en-us) AppleWebKit/525.18 (KHTML, like Gecko) Version/3.1.1 Safari/525.20
Build Identifier: 

Title says it all. Make sure nanojit and the tracing engine have a well defined interface to the VM core in order to allow them to be used with other JavaScript VMs.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Andreas Gal :gal]

Attachment: Don't assume nanojit is the default namespace and remove unnecessary include.

Comment 2

[Edwin Smith]

Comment on attachment 325792 [details] [diff] [review]
Don't assume nanojit is the default namespace and remove unnecessary include.

patch looks like just two lines changed, is that really all?

Comment 3

[Andreas Gal :gal]

These are the must-haves I couldn't work around with a custom avmplus.h. 

I will try to get the debug code to run next, that will require a bigger patch.

Last step will be cleanup. Some stuff is in the global name space that shouldn't be there and such.

Breaking out the tracer will be a lot more difficult. Nanojit seems pretty well compartmentalized.

Comment 4

[Rick Reitmaier]

So should we wait and accumulate the changes to one big patch then?

Comment 5

[Andreas Gal :gal]

Ok, we landed nanojit in tracemonkey. Basically I can work around pretty much everything with some nasty glue code. If you look at the glue you can see which parts should be refactored:

http://ssllab.org/hg/tracemonkey/file/d7f7e6fe6ebe/js/src/nanojit/avmplus.h

I think all references to AvmCore should go and nanojit should have its only configuration data structure (eliminating AvmConfiguration). The allocation code is Fragmento is somewhat ugly. I think Rick mentioned he wants to improve that anyway (GCHeap). I think we can keep class GC in general , thats easy to mimic. core->console is used only in one spot. A nicer interface without the stream syntax would be probably an improvement. OSDep::getDate() gets called somewhere. That could move somewhere else I assume.

Most of this looks like very small changes to me. Any comments?

Comment 6

[Steven Johnson]

All those changes sounds spot-on to me. 

Comment 7

[Rick Reitmaier]

I like the idea of having a nanojit config structure (it should probably reside in Fragmento; maybe time for a name change?)  I think we should be able to create a MemMgr interface that we can use to tie into the GC (or some other mem manager) pretty cleanly.  We should also probably move the _pages related code out of Fragmento into a helper class that interfaces with MemMgr.

One thing I've been thinking about is to move the page linked-lists off page.
That is, have the list reside in an external data structure.  This would mean that we could use the GC for collecting the LIR and code pages.  It also means that we could share code pages with different fragments.   I'm not sure whether this would make it harder or easier for your work.

Comment 8

[Andreas Gal :gal]

A MemMgr sounds great. That would definitively clean up our glue code. We will not be collecting any parts of TT through our GC, but if the MemMgr gets told when pages go dead we can do some thin reference counting on them in our code base so I am all for it.

Comment 9

[Edwin Smith]

Comment on attachment 325792 [details] [diff] [review]
Don't assume nanojit is the default namespace and remove unnecessary include.

pushed, too.  trivial change.

Comment 10

[Andreas Gal :gal]

Wishlist in order of preference. To vote on the order just copy the list and re-order and say why you want it that way. Things that help tamarin primarily should go first, followed by stuff that helps other users but also cleans up tamarin, and user specific stuff should go last (IMO).

Big items (takes time):

1. SideExit restructuring. Create a mechanism to attach VM-specific information to a side exit. Ideally this should also be used to eliminate most of the current information in struct SideExit. IMO, the VM knows how to side exit and recover its interpreter state. Nanojit should only do the basics and spot the VM. Updating pointers (sp_adj, pc_adj) etc seems more efficient when done from C, but the trace has to provide/store the information in the LIR. The side exit could return the address to a VM-custom buffer embedded in the LIR that tells the side exit handling code in the VM how to fix up the state. This will reduce the code size of traces so TT might win as well.

2. Clean way to attach VM-custom data to the entry point of a tree. Our trees are type-specialized. This data has to be attached to the tree and should be stored in the LIR (LIR_skip). TT might do similar things and could use a similar mechanism.

4. Move tracker and load/store elimination into nanojit.

5. Move state reconstruction at side exits into nanojit. Depends on (1).

Little things (one liners and such):

1. Add operator (gc*) delete to avmplus.h so we can start doing explicit deallocation in nanojit (we will provide the patch to add the deallocations over time).

2. Move verbose filter into nanojit.

Comment 11

[Andreas Gal :gal]

Attachment: Don't assume nanojit is the global name space.

If you guys want to make nanojit the global name spaces I would be ok with that too. We just have to make a decision and stick to it :) 99.9% of the code does not depend on it and uses explicit namespaces, thats why I keep harping on this one (and now 2) case(s).

Comment 12

[Andreas Gal :gal]

Attachment: Don't assume nanojit is the global name space.

Previous patch was reverse. This one should apply directly.

Comment 13

[Edwin Smith]

Another little thing

* give nanojit its own configuration object, dont depend on core

Comment 14

[Edwin Smith]

Attachment: ns fixes, move VerboseFilter->nanojit, add insImmPtr and use it.

also subsumes attachment 326522 [details] [diff] [review] in bug 433793

Comment 15

[David Anderson [:dvander] - inactive, e-mail if emergency]

Looks like the patch didn't attach right again

Comment 16

[Edwin Smith]

Attachment: ns fixes, move VerboseFilter->nanojit, add insImmPtr and use it.

fixed the patch

Comment 17

[Andreas Gal :gal]

Comment on attachment 326700 [details] [diff] [review]
ns fixes, move VerboseFilter->nanojit, add insImmPtr and use it.

Looks great.

Comment 18

[Andreas Gal :gal]

Attachment: Add a VM-private field to Fragment.

Please review. I will push if +.

Comment 19

[Andreas Gal :gal]

Attachment: Add a vmprivate field to guard record.

This is the minimal infrastructure I need to hook in my typemaps in case the big patch can't land.

Comment 20

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 327728 [details] [diff] [review]
Add a VM-private field to Fragment.

pushed

Comment 21

[Andreas Gal :gal]

Attachment: Pass ip for fragment lookup instead of entire InterpStruct

Should not affect code size. Cleans up the interface in Fragmento and makes it independent of InterpStruct, which is VM-specific. masonchang will push to sandbox and to tree if approved.

Comment 22

[Edwin Smith]

Comment on attachment 327876 [details] [diff] [review]
Pass ip for fragment lookup instead of entire InterpStruct

is FOpcode vm specific?  i think so.  i've used const void *.  either is fine with me but if you agree, just change the interface to const void* instead of FOpcode* 

I also think FragID is obsolete at this point, could be optionally cleaned up.  (frid is just an address like ip).

Comment 23

[Andreas Gal :gal]

I will redo the patch with void* and submit a separate one for review to remove fragid.

Comment 24

[Andreas Gal :gal]

Attachment: Pass ip for fragment lookup instead of entire InterpStruct. FOpcodep is now simply const void*. fragment->frid renamed to fragment->ip.

Comment 25

[Andreas Gal :gal]

Mason, if ed reviews the two pending patches favorably, please push to sandbox and then to the repository if there are no issues.

Comment 26

[Edwin Smith]

Comment on attachment 327865 [details] [diff] [review]
Add a vmprivate field to guard record.

this is fine, i want to unblock you, we can still strive for cleanup as we go.

Comment 27

[Mason Chang]

Attachment: VM Private field + pass IP patches rolled into one. Changes so DEBUG builds work.

Comment 28

[Andreas Gal :gal]

Looks good. Thanks mason. Assigning to david to push into TT.

Comment 29

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 328036 [details] [diff] [review]
VM Private field + pass IP patches rolled into one. Changes so DEBUG builds work.

pushed as changeset 59a1b730661a

Comment 30

[Andreas Gal :gal]

Attachment: Rename POP() to POPr().

This renames POP() to POPr(), which is more consistent (IMO), and at the same time avoids a name clash in tracemonkey.

Comment 31

[Andreas Gal :gal]

Comment on attachment 328087 [details] [diff] [review]
Rename POP() to POPr().

I can work around it differently if needed, but this seems to be a general clarification. Let me know what you think.

Comment 32

[Andreas Gal :gal]

Mason, please sandbox https://bugzilla.mozilla.org/attachment.cgi?id=328087 and assign it back to me when its ready to be pushed. Thanks.

Comment 33

[Andreas Gal :gal]

Attachment: Follow chains of trampolines (allows LIR_tramp to be used to kill instructions alread emitted).

Comment 34

[Andreas Gal :gal]

https://bugzilla.mozilla.org/attachment.cgi?id=328087 pushed as

http://hg.mozilla.org/index.cgi/tamarin-tracing/rev/1d0b430e7410.

Comment 35

[Andreas Gal :gal]

Mason, you know the drill. I will try to find out whats happening to your repository access request so you can push it yourself after sandboxing it.

Comment 36

[Brendan Eich [:brendan]]

Just FYI, we find one bug per landed patch much better for back-out and hg blame issue tracking and general project management hygiene. More of a pain, until the pain of overloading one bug with many landed patches bites. YMMV, let me know.

/be

Comment 37

[Andreas Gal :gal]

Ed suggested one bug to track these changes. I don't mind separate reports. Would facilitate workflow. I just assign them to mason when we are ready to push and he sandboxes, pushes and closes it. As long Ed is ok with it I am game.

Comment 38

[Edwin Smith]

works for me.  we could move the todo list to tamarin wiki.

Comment 39

[Andreas Gal :gal]

Ok, mason when you are done testing & pushing, please close this bug.

Comment 40

[Andreas Gal :gal]

Pushed by mason as http://hg.mozilla.org/index.cgi/tamarin-tracing/rev/c47936dce08a

As per brendan closing this uber bug and we will open individual ones for future issues in this area.