543589 - AvmCore::current() would be handy.

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, enhancement)

Description

[Krzysztof Palacz]

A lot of gymnastics is often involved in getting a reference the current AvmCore.

Comment 1

[Krzysztof Palacz]

Attachment: Impl. as per Tommy's suggestion.

Comment 2

[Tommy Reilly]

Could we introduce a GC::current and have AvmCore::current use that?   A little too much MMgc guts bleeding into AvmCore for my liking.

Comment 3

[Tommy Reilly]

Comment on attachment 424670 [details] [diff] [review]
Impl. as per Tommy's suggestion.

+1 with the caveat that GCHeap::GetGCHeap()->GetEnterFrame()->GetActiveGC() expression becomes GC::current() and moves to the GC.

Comment 4

[Krzysztof Palacz]

Attachment: New and improved, now with GC::current

Comment 5

[Lars T Hansen]

Note that I get to SR any and all API changes to the GC, and I'm not likely to SR+ a new method with a name as generic as "current".

Comment 6

[Tommy Reilly]

Lar's probably wants something like:

static GC *GetCurrentActiveEnterFrameThreadLocalGC() 

;-)

Comment 7

[Lars T Hansen]

(In reply to comment #6)
> Lar's probably wants something like:
> 
> static GC *GetCurrentActiveEnterFrameThreadLocalGC() 
> 
> ;-)

Actually Lars doesn't quite understand why current() is a method on GC, since the threadlocal (currently hidden inside GCHeap) is just a global variable anyway.  But sure, why not.

GC::GetActiveGC is probably more along the lines of what Lars was looking for.

(Lars know what you're thinking, he would call it get-active-gc if he could.  And indeed he would.)

Comment 8

[Lars T Hansen]

Let me rephrase that.  If the threadlocal is hidden inside GCHeap, and the logic for the enterframe stuff is tied to the GCHeap, then the getter for the current GC really belongs on the GCHeap, no?

Comment 9

[Krzysztof Palacz]

Not knowing anything about it, I'd look for a way to get the current instance of GC among the methods of GC (much like Thread::current(), a common idiom).

Comment 10

[Tommy Reilly]

That the current implementation piggy back's on the GCHeap's thread local EnterFrame is an implementation detail to me that could change and having a static API in GC feels "right" to me.    Actually I think it would be cleaner for the GC to have its own thread local but its best not to have too many thread locals.

Comment 11

[Steven Johnson]

I have to say the concept of this makes me nervous, as it's effectively taking a global variable access and making it appear not to be one. If we find that we frequently need this global variable access, well, eek.

Comment 12

[Edwin Smith]

Perhaps this bug and the api should be motivated by what AvmCore::current() is for.  In an email conversation, Krzystof mentioned that its handy for debugging, when you're stopped in the code and don't have a pointer to an object that you can get the AvmCore* pointer from.  if thats the use case (and nothing more) then the api and comment-documentation should make that clear.  and sure, if its handy and not fundamentally broken or unmaintainable, cool.

The other potential extreme is a general replacement for foo->core(), or passing AvmCore* around as an argument.  This may also be a fine proposal (its not unheard of to have at least one TLS "context" variable, at least for allocation).  But thats more ambitious and justifies more rigor, for example: whats the plan to refactor all the code?  who will set it on vm entry ? what is the performance of TLS variables on various platforms? what is the testing strategy? etc.

Comment 13

[Krzysztof Palacz]

Hmm, the way I see it, it's the converse: AvmCore is not a global variable (many instances can coexist in a single program), and we make it nearly as easily accessible as a global variable. 
I don't think the addition would violate the principle of least authority (POLA) any more than the existing codebase already does, because nearly every method already has access to a reference to AvmCore, and if it doesn't, it'll need it soon enough to access the console to print debugging info.
If we are concerned about POLA, then AvmCore should be broken up into smaller classes.

Comment 14

[Rick Reitmaier]

re comment 12:  Agreed; Krzystof's example of being able to call DebugCLI::bt() and have it display an AS stack from within a gdb prompt, is convincing enough to allow it in debug shell/players.

Comment 15

[Tommy Reilly]

We currently rely on the EnterFrame thread local's GC field in a couple different places, OOM and the Sampler for one and if you try to allocate any GC memory without setting it we assert.   So this mechanism can be relied upon.  This bug is mostly about cosmetic issues and where API's should live as I see it. 

It might not be the most performant API, we use the generic OS thread local API's and don't attempt to use the tricky fast ones that certain compilers support.

Comment 16

[Krzysztof Palacz]

Attachment: New and improved, now with a better name.

Comment 17

[Lars T Hansen]

Comment on attachment 424922 [details] [diff] [review]
New and improved, now with a better name.

Speaking for myself I'm happy with the GC change, though I don't know why the other function shouldn't be called AvmCore::GetActiveCore or somesuch.  Leaving that up to Ed though.

Comment 18

[Edwin Smith]

Comment on attachment 424922 [details] [diff] [review]
New and improved, now with a better name.

lets go for self-consistency, AvmCore::getActiveCore(), R+ from me with that tweak.

cool feature, btw.

Comment 19

[Krzysztof Palacz]

Attachment: final rename

Comment 20

[Rick Reitmaier]

pushed http://hg.mozilla.org/tamarin-redux/rev/892afc2885e4