Fall back to interpreter when codegen hits 64k limit and fails. #76

Open
wants to merge 1 commit into
from

Projects

None yet

3 participants

@headius

Currently, if optimize level is set to >= 0, it is possible for code to fail to run at all when it exceeds the JVM's 64k bytecode size limit. This patch (a work in progress) modifies Codegen to fall back to Interpreter when any errors occur during code generation.

I recognize that this catch-all is not ideal, but I'm posting this now to start a discussion. It is not clear to me why having code fail to execute is better than falling back to interpretation, but the hard break at the 64k limit means most applications of Rhino set optimize level to -1. I believe this has harmed the world's impression of Rhino and its performance.

This could also be an optional setting, enabled by default but possible to disable for those who really want code >64k to blow up. I'm not sure who those people are.

@headius

Oh, and I did not run Rhino tests, but I did test with a very large script to prove this works.


system ~/projects/rhino $ wc -l giant.js 
   15649 giant.js

system ~/projects/rhino $ java -jar build/rhino1_7R5pre/js.jar -opt 9 giant.js 
js: "giant.js", line 1: Encountered code generation error while compiling script: generated bytecode for method exceeds 64K limit.


system ~/projects/rhino $ git stash pop ; a jar
# On branch master
# Changes not staged for commit:
#   (use "git add <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#   modified:   src/org/mozilla/javascript/optimizer/Codegen.java
#
# Untracked files:
#   (use "git add <file>..." to include in what will be committed)
#
#   .idea/
#   Rhino.iml
#   build/
#   giant.js
#   lib/
no changes added to commit (use "git add" and/or "git commit -a")
Dropped refs/stash@{0} (a3cfc10bff2dbacfe6aa1554f3fbd29c16dfd026)
    [javac] warning: [options] bootstrap class path not set in conjunction with -source 1.5
    [javac] 1 warning
     [echo] Calling /Users/headius/projects/rhino/xmlimplsrc/build.xml
     [echo] Compiling XMLBeans E4X implementation using ./lib/xbean.jar and ./lib/jsr173_1.0_api.jar

BUILD SUCCESSFUL
Total time: 2 seconds

system ~/projects/rhino $ java -jar build/rhino1_7R5pre/js.jar -opt 9 giant.js 
2
3
4
5
6
7
8
@hns
hns commented Aug 9, 2012

We should definitely do this, given how easy it is to implement and how much it improves the user experience.

One problem with the proposed patch is that by catching all exceptions it will mask actual bugs in bytecode generation. I think the better solution is to let Codegen throw the more specific ClassFileFormatException without converting it to a generic runtime exception, and implement the interpreter fallback in Context.compileImpl().

I'm working on a new patch. I'm also considering to add a new FALLBACK_TO_INTERPRETER context feature, but the default setting would be true (as you said, why would we want to fail by default).

@szegedi
Mozilla member
@headius
@hns
hns commented Aug 9, 2012

Attila, my thinking was that in some cases it might be nice to have a guarantee that code actually runs with the dialed settings. Probably not for the common case of getting things done, but for example for running our own tests.

Charlie, I think I've got it pretty much right locally so no big need to work on it (unless you really want to contribute a commit, which would be nice :)

One annoyance I found is we can't use ClassFileWriter.ClassFileFormatException in Context because ClassFileWriter is not available in the smalljs.jar target (which is an interpreter-only packaging of Rhino). Adding a org.mozilla.javascript.ClassLimitException and make ClassFileFormatException.ClassFormatException extend it... yuck!

@hns

I pushed my revised patch to my github fork:

hns@207bb86

As mentioned in my previous comment, the introduction of the ClassLimitException is due to jar packaging considerations (making Context class work without src/org/mozilla/classfile package).

Please review and comment!

@headius

Is there anything outstanding to get @hns's version of the patch into rhino proper?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment