471164 - Generated NativeFunction override methods are suboptimal

Status: RESOLVED INACTIVE

(Rhino Graveyard :: Compiler, enhancement)

Description

[Cameron McCormack (:heycam)]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.2a1pre) Gecko/20081225 Minefield/3.2a1pre
Build Identifier: 

The NativeFunction override methods that are generated in o.m.j.optimizer.generateNativeOverrides() produce methods that use more code than they need to.  Specifically, there is a lot of repetition of 'const/return' opcode pairs.

The worst case I find in my compiled code is in getParamOrVarConst().  Since my code doesn't use const at all, the method will return false regardless of the function ID and param/local index passed in to it.  The generated method, however, uses nested switches for each possible function ID and param/local index combination, where each case just returns false anyway.


Reproducible: Always

Comment 1

[Cameron McCormack (:heycam)]

For example, here is the largest js file that I compile in one of my projects:

  http://mcc.id.au/temp/2008/EditCanvas.js

After compiling, you can see with javap that the method takes 4710 bytes of code, when in fact just two bytes are really required (iconst_0 / ireturn).

Comment 2

[Cameron McCormack (:heycam)]

Attachment: Patch to optimize the generated NativeFunction override methods

This patch optimizes the generated methods somewhat.  For the methods that take a param/local index argument (like getParamOrVarConst()), there are 5 forms the generated method will take:

  * In the worst case, every combination of function ID and param/local index
    returns a different value.  The generated method uses nested switches
    that all jump to a different location to return the relevant value, just
    like the current code in CVS.  A small optimization is done in this case,
    though: the iload_1 instruction is generated at the top of the method,
    instead of being done before each of the inner tableswitches.

  * A slightly better case is when some of the combinations of function ID and
    param/local index return the same value.  In this case, only a single
    load/return instruction pair will be generated, and the various switch
    cases that should result in this value being returned all jump to this
    single load/return pair.

  * Even better is if all of the values for a given function return the same
    value, regardless of the param/local index.  In this case, an inner switch
    won't be generated, and the outer switch will jump straight to the relevant
    load/return pair.

  * If the same value is returned regardless of the
    function ID and param/local index, then the generated method consists just
    of a single load/return instruction pair.

  * Finally, if the same value is always returned, and this value is the same
    that is returned from the overridden method, then we can avoid generating
    the method altogether.  (This is the case for getParamOrVarConst() if it
    always returns false.)

The generation of the native methods is factored out into a base class,
NativeFunctionOverrideGenerator, which decides which of these five forms the method will take and generates most of the method.  The subclasses handle the different return types of the override methods, and look at the values that are returned for all of the function ID and param/local index combinations to determine how much overlap there is.

Comment 3

[Justin Dolske [:Dolske]]

Comment on attachment 354473 [details] [diff] [review]
Patch to optimize the generated NativeFunction override methods

Clearing up old unassigned bugs. Not sure what to do with this one... Waldo, can you reassign (or close) as appropriate? Thanks.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 354473 [details] [diff] [review]
Patch to optimize the generated NativeFunction override methods

I'm going to cancel this at the very least because it's likely horribly bitrotted.  Also, Rhino development now appears to occur on Github, so if this is desired, it should probably be brought up there.  (I don't have an account there, so I can't bring it up myself.)

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Closing. Bug management is now done here:
https://github.com/mozilla/rhino