618123 - MethodSignature contains method implementation details.

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Virtual Machine, enhancement, P3)

Description

[Krzysztof Palacz]

Typically a method signature is used together with a name to uniquely identify a method. However, avmplus::MethodSignature also contains the details of this method's implementation in bytecode (e.g., number of locals or maximum scope stack size). These details are  needed to verify, interpret or compile the method, but not to refer to it. This conflation of roles can result in conceptual confusion, but it also ties together fragments of state with different lifecycles. The implementation details are only needed if a method is actually executed, and then only until its jit compilation (unless deoptimization to interpreted mode happens, in which case the implementation details can be reparsed from the source bytecode).
On the other hand, a MethodSignature is needed as long as its method can potentially be invoked, which is usually for the duration of the program.

Comment 1

[Krzysztof Palacz]

Attachment: Implementation details factored out of MethodSignature

Gauging interest in continuing this work. 
Implementation details are factored out into a separate stack-allocated MethodBodyInfo class. Currently MethodBodyInfo is recreated on every interpreted invocation, which is fine in the default exec policy but could be problematic for a wordcode interpreter (and possibly OSR). If this is indeed a problem, the MethodBodyInfo could be retained until jit compilation (if it ever happens).

Comment 2

[Krzysztof Palacz]

(Re: comment #0)
Also, without implementation details, method signatures could be shared, at least between the base method and its overriding methods.

Comment 3

[Edwin Smith]

Comment on attachment 496660 [details] [diff] [review]
Implementation details factored out of MethodSignature

Its a good cleanup; separating data with different lifetimes is good, as is stack allocating this small struct.  (RIAA ftw!)

Is this just general cleanup or does it help with something coming after it?

Note that MethodSignature instances are cached by a QCache, and so have bizarre lifetimes.  Interning them could be interesting -- in a big application, there are many more actual methods than unique signatures.  If that were done, would we still benefit much from the QCache?  

On the next round, get feedback from Bill; some of this might interact with OSR.  If you pursue the QCache experiment, get feedback from Steven.

Comment 4

[Lars T Hansen]

What made you so gung-ho for IPR?... I bet you have a magtape at home with the Unix v6 sources - watch it!

Comment 5

[Edwin Smith]

man, just searching for RIAA to make sure i was using & spelling it right, was a bear.

Comment 6

[Krzysztof Palacz]

(In reply to comment #3)
> Is this just general cleanup or does it help with something coming after it?
> 
> Note that MethodSignature instances are cached by a QCache, and so have bizarre
> lifetimes.  Interning them could be interesting -- in a big application, there
> are many more actual methods than unique signatures.  If that were done, would
> we still benefit much from the QCache?  

Occasional, unpredictable allocations while doing something as seemingly innocuous as, say, retrieving the declaring traits of a MethodInfo, seems to be a recipe for pain from concurrency's standpoint, given that the allocation may result in a safepoint, and the safepoint task may need whatever locks happen to be held by the thread that triggered the allocation. Obviously, this is not the end of the world, but the fewer situations of this sort we have to worry about, the better (esp. since both current and future Player code may be affected). 
So yes, getting rid of the QCache here would be the optimal outcome of this refactoring (and would improve const-ness, as well).

Comment 7

[Steven Johnson]

(In reply to comment #6)
> So yes, getting rid of the QCache here would be the optimal outcome of this
> refactoring (and would improve const-ness, as well).

Just be sure to measure the memory effects; the QCache scheme was introduced as a nontrivial memory savings, which we don't want to backtrack.

Comment 8

[Krzysztof Palacz]

(In reply to comment #7)
> Just be sure to measure the memory effects; the QCache scheme was introduced as
> a nontrivial memory savings, which we don't want to backtrack.
Certainly.

Comment 9

[Krzysztof Palacz]

Attachment: rebased to r5712, removed MethodBodyInfo::_isNative, moved handling of frame size padding to the interpreter

MethodBodyInfo is not created for native methods, so one less field needed.
The padding of frame size is platform and execution mode specific, and was moved to Interpreter.cpp (MethodBodyInfo reflects exactly the bytecode or wordcode definition). 
Requesting Bill's feedback, esp. regarding interaction with OSR.

Comment 10

[William Maddox]

It looks like there's a nontrivial amount of work required to construct a MethodBodyInfo object.  We should avoid duplicating this at each invocation of the interpreter.  Under OSR, we may interpret many times before compiling.  Also, we should not make running interpreter-only any slower than it is inherently.

I'm wondering if the content of MethodBodyInfo couldn't be moved to the MethodInfo object.  The MethodBodyInfo fields would be unused for native methods, which would be unfortunate.  Upon a superficial examination, however, it does look like a MethodInfo object gets passed to the places where we need access to MethodBodyInfo members.  While I'm not necessarily suggesting such a refactoring, wouldn't it be architecturally appropriate for MethodInfo to have subclasses NativeMethodInfo and ABCMethodInfo?

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.