264637 - InterpretedFunction memory footprint could be lighter

Status: RESOLVED FIXED

(Rhino Graveyard :: Core, enhancement)

Description

[Attila Szegedi]

User-Agent:       Opera/7.54 (Windows NT 5.0; U)  [en]
Build Identifier: 

The boolean flag "evalScriptFlag" in the class InterpretedFunction could be 
moved to InterpretedData. This would make InterpretedFunction (of which one 
instance is created per declared function per script execution) lighter, on 
expense of InterpretedData (of which one instance is created per declared 
function per script parsing).

With support for continuations in 1.6, it is also important to be able to stub 
out as much of the objects representing an interpreted function as possible when 
serializing interim state, and reconnect to the already present shared instances 
upon deserialization. It is therefore important to push as much of the immutable 
state as possible to InterpreterData class - I will likely follow up with 
proposals for further reductions in InterpretedFunction's base class, 
NativeFunction. 

Reproducible: Always
Steps to Reproduce:

Comment 1

[Attila Szegedi]

Attachment: patch for moving evalScriptFlag from InterpretedFunction class to InterpreterData class

Comment 2

[Igor Bukanov]

I committed the patch: it is sufficiently simple not to cause any problems.

Comment 3

[Igor Bukanov]

Attachment: Patch to remove NativeFunction.(version|argNames|argCount)

The patch replaces version|argNames|argCount fields in NativeFunction by
getLanguageVersion()|getParamCount()|getParamAndVarCount()|getParamOrVarName(int

index) abstract methods.

For the interpreted mode the methods are then trivially implemented in
InterpretedFunction using information stored in InterpreterData. 

For the class compilation mode patch adds code to generate methods'
implementations using data embedded in the generated class file. In this way
for the class compiler the patch not only removes 3 fields from each function
instance but also avoids the construction of argNames array replacing the
bytecode to initialize it by the bytecode to do the switch over var index. 

The patch also replaces extending from NativeFunction in
NativeScript|NativeRegExpCtor classes by the proper extending from BaseFunction

which was a oversight left from pre-1.5R5 reorganization of code generation.

Comment 4

[Attila Szegedi]

(In reply to comment #3)
> Created an attachment (id=162381)
> Patch to remove NativeFunction.(version|argNames|argCount)
> 

Wow - this indeed sounds as a great improvement. 

A coworker who's intensively working on JavaScript apps will probably kick in 
later today to work with it, so any glaring bugs should become apparent rather 
quickly. I myself have been running some tests with it, and so far it looks 
okay.

Now, risking that I'll come across as becoming too greedy, I assume 
"functionName" in BaseFunction could also undergo a similar treatment of being 
replaced with an abstract getFunctionName() method, couldn't it? The function 
name is either calculable from other data (FieldAndMethods, overloaded case of 
NativeJavaMethod*, NativeJavaConstructor, InterpetedFunction) or fixed 
(NativeRegExpCtor) in lots of subclasses.

[*] Of course, it also has a constructor with explicit name, but that could 
really be handled by a separate subclass named "NamedNativeJavaMethod" or 
something similar - note the case of NativeJavaMethod that takes Method[] as 
argument (and by inference, all instances of FieldsAndMethods class) don't need 
the functionName field as they directly use the Java method name as their name.

Comment 5

[Igor Bukanov]

Attachment: Patch to replace BaseFunction.functionName by BaseFunction.getFunctionName()

Comment 6

[Igor Bukanov]

The patch was committed some time ago.