[rhino] do not force therubyrhino (>= 2.0.2) into interpreted mode #112

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
4 participants

kares commented Dec 5, 2012

Hey, I've just released therubyrhino 2.0.2 which deals with the 64K limit issue itself.
It would be great to get this into an ExecJS release since it provides a ~30% performance increase e.g. when compiling assets on JRuby.

It might not work for all and might even de-crease performance but only with about a second for every script that fails compiling into a byte-code and thus gets re-tried in (slow) interpreter mode.

These are scripts that have a lot of code in a single scope (e.g. in global) and since each scope gets compiled into a method it is most likely to fail, one such script is coffee-script.js and the de-crease might be observed in tests :

therubyrhino 2.0.1

kares@theborg:~/workspace/github/execjs$ rake test:rubyrhino
Run options: 

# Running tests:

.................

Finished tests in 3.825000s, 4.4444 tests/s, 15.9477 assertions/s.

therubyrhino 2.0.2

kares@theborg:~/workspace/github/execjs$ rake test:rubyrhino
Run options: 

# Running tests:

...[INFO] Rhino byte-code generation failed forcing org.mozilla.javascript.Context@5fadce into interpreted mode
..............

Finished tests in 5.145000s, 3.3042 tests/s, 11.8562 assertions/s.

But this is not really an issue since when compiling multiple-scripts a second wasted on a script or two might be re-gained on others (and if it's a single huge script it will only add-up ~1s once). The latest gem also allows to specify optimization level via a system property thus strict (interpreter) Rhino users that do not trust Java byte-code will still be HAPPY :) !

Now let's see the gains of this addition with pre-compiling assets Rails 3.2.9 on JRuby with :
environments/production.rb

  config.assets.compress = true
  config.assets.digest = true

application.js contains the "usual" jQuery files :

//= require jquery
//= require jquery
//= require jquery_ujs
//= require jquery.ui.all
//= require jquery.mobile-1.1.1

BEFORE (therubyrhino 2.0.1) :

kares@theborg:~/workspace/github/jruby-rack/examples/rails32$ time rake assets:precompile:all RAILS_ENV=production

real    2m55.666s
user    2m54.811s
sys 0m2.164s

AFTER (therubyrhino 2.0.2) :

kares@theborg:~/workspace/github/jruby-rack/examples/rails32$ time rake assets:precompile:all RAILS_ENV=production

real    2m0.239s
user    2m1.424s
sys 0m2.136s

We seem to have saved 1 minute out of 3 which should be noticeable by all JRuby users ...

kares added some commits Dec 5, 2012

@kares kares do not force therubyrhino (>= 2.0.2) into interpreted mode
as the latest gem release 2.0.2 has support for dealing with Rhino's the 64K byte-code method limit ... it will fallback to -1 (and retry)
maintained backwards-compatibility with older therubyrhino gem versions as well as ExecJS "API" compatibility - user might still send(:fix_memory_limit!)
c294d91
@kares kares OrgMozillaJavascript::NativeFunction is a Rhino::JS::Function
... also use the same name-space convention for all
b57e5c5
Owner

sstephenson commented Dec 5, 2012

Is there a way we can feature-test the need to call fix_memory_limit! without comparing the gem version? Your change introduces a dependency on RubyGems.

@kares kares do not depend on rubygems API to test for the "memory limit fix"
since there's no public API for the "memory limit fix" (actually it's a byte-code limit fix) we test for the default_optimization_level which was added in the same release therubyrhino-2.0.2
706042f

kares commented Dec 5, 2012

Right, I did not want to test for private methods / constants added due that feature since they might go away (the "fix" to downgrade to interpreted mode is also reported on Rhino itself - although I do not expect them to do releases anymore since they're rewriting it in German :)). But we can test for another feature added in the 2.0.2 release - updated ...

kares referenced this pull request in cowboyd/therubyrhino Dec 5, 2012

Closed

Evaluate Rhino optimization levels #23

Owner

sstephenson commented Dec 5, 2012

That isn't any better -- what happens when Rhino removes default_optimization_level in a hypothetical future update? We don't want to test against the version, or some feature that was introduced in that version. We want to test for the behavior we're targeting.

Perhaps there's a way to trigger the 64K limit and rescue from it?

kares commented Dec 5, 2012

I do leave it up to you guys than to decide ... default_optimization_level is at least public API that shall stay.
You can check for the private method code_generation_error? if it's there cowboyd/therubyrhino@05ece36 but that's not public API since all the functionality might get to Java code one day (mozilla/rhino#76) ... You can trigger the 64K limit by evaluating a script e.g. a one that has ~20.000 var statements (see the spec in the linked commit) but knowing for sure it's the "64K limit" exception is a bit tricky since ... well Rhino is doing a good job of hiding it (it's not custom typed, the exception message is localized + similar exceptions related to byte-code limits might be thrown) ...

I'm sorry it's "poor" code but somehow this feature is meant to be "completely transparent" and I would certainly not want to pollute ExecJS with more "poor" code but I can't think of other options here.
In the end it's only due backwards compatibility it can go if we're sure to have gem 'therubyrhino','>= 2.0.2'.

Speaking of assumptions ExecJS currently does a "poor" one about Rhino's API (see my other commit in this pull) - assuming all functions are an instance of NativeFunction.

Owner

sstephenson commented Dec 5, 2012

FWIW I'm totally in favor of this change, and I appreciate the pull request. We just need to figure out how to condition it in a non-brittle way.

kares commented Dec 6, 2012

Right, me too :) ... I meant the lengthy explanation for you guys, cause I really hate eval-catching a "big" JS for an error.
And am a bit out of other "good" options to detect this ...

atambo commented Jan 2, 2013

Why not just bump ExecJS's dependency on therubyrhino to >= 2.0.2?

josh closed this Aug 21, 2013

kares commented Aug 21, 2013

@josh thanks for closing ... but what is the outcome do you guys simply keep forcing rhino into interpreted mode, why?

kares referenced this pull request in cowboyd/therubyrhino Dec 6, 2013

Closed

Performance / memory leak? #24

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