Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bad performance with jshint v1.1.0 #43

Closed
ifurnadjiev opened this issue Mar 21, 2013 · 9 comments
Closed

Bad performance with jshint v1.1.0 #43

ifurnadjiev opened this issue Mar 21, 2013 · 9 comments

Comments

@ifurnadjiev
Copy link
Member

The performance with the latest JSHint Eclipse v0.9.6 (build-in jshint v1.1.0) is very bad. Using the previous version of jshint (jshint-r12.js) with the latest JSHint Eclipse v0.9.6 solves the problem.
Some numbers using RAP org.eclipse.rap.rwt.jstest bundle and cleanup of the project:

  • jshint v1.1.0 - bundle rebuild took 148 sec with 99 warnings
  • jshint r12 - bundle rebuild took 19 sec with the same 99 warnings
    Almost 8 times worse performance.
@shunter
Copy link

shunter commented Mar 21, 2013

I'm seeing this performance problem too after updating. I did some cursory profiling with VisualVM and found it was spending all its time in org.mozilla.javascript.InterpretedFunction.call(). Was Rhino optimization inadvertently turned off with the Rhino version upgrade?

@ralfstx
Copy link
Member

ralfstx commented Mar 21, 2013

Thanks Ivan, I can reproduce the problem, and I can confirm that replacing jshint 1.1.0 with r12 cures the problem. So it seems to be related to the new jshint version. I think we should verify these results with Rhino on the command line and raise a jshint issue if it's reproducible there too.

For the moment, the workaround is to downgrade jshint to an older version.

@ralfstx
Copy link
Member

ralfstx commented Mar 21, 2013

shunter, I have to admit that I'm not overly familiar with tuning Rhino. I've tried context.setOptimizationLevel( 9 ) but that didn't seem to make any difference. If you know how to the performance on the JSHint class can be improved, your advice is welcome.

@shunter
Copy link

shunter commented Apr 25, 2013

I've been following the progress of this performance issue, and it does seem clear now that it is the changes to JSHint itself that causes the problem, since 1.1.0 evals all of the JSHint code, preventing optimization regardless of what optimization level is set on Rhino. I wanted to find out if the latest head version of JSHint fixes the issue, but after cloning and building JSHint from Git, I get "File is not a valid JSHint library" when I try to configure the Eclipse plugin to use the version I built. Is there a problem with the detection code that makes it incompatible with the latest JSHint? I tried running my built version on the command line with Rhino 1.7R4 and runs correctly there.

@ralfstx
Copy link
Member

ralfstx commented Apr 26, 2013

Great that you give it a try! I was curious about the fix as well but somehow didn't succeed building jshint.

The error should indicate that no global function named "JSHINT" has been found. Could you check if this is present? Could you share your jshint build?

@shunter
Copy link

shunter commented Apr 26, 2013

Here's the version of JSHint that I built from the Git repo.

https://gist.github.com/shunter/5469064/raw/da3e7358f78c03a58b5b31cdf8965c323f8ab04a/jshint-1.1.0.js

It doesn't say anything in the error message about a global JSHINT function. I traced the error message to here, which is in a catch (Exception) block which doesn't include the exception message in the text. Perhaps you can debug the plugin code to see what exception is being thrown?

@ralfstx
Copy link
Member

ralfstx commented Apr 28, 2013

Thank you. The exception is: org.mozilla.javascript.EcmaError: ReferenceError: "window" is not defined. (jshint library#4820). It seems to be related to the function starting at line 4820, which tries to get the console object from window. I get the same error with Rhino 1.7R4:

java -jar rhino1_7R4/js.jar jshint-1.1.0.js
js: uncaught JavaScript runtime exception: ReferenceError: "window" is not defined.

Opened jshint/jshint#1038. In the meantime, adding an empty window object before evaluating the library avoids the exception, this could be a workaround.

The executions time is much better. It still seems to be slightly above r12, but I'd say the difference is less than 5%.

@shunter
Copy link

shunter commented May 1, 2013

I see your JSHint issue was closed, so I think your workaround will end up being the right approach. The JSHint build does also produce a "rhino" version, which has added to the top:

#!/usr/bin/env rhino

var window = {};

This version doesn't work with the Eclipse plugin either because of the #! stuff at the top, but it does show that an empty window object is the right approach for Rhino. I see you're already adding fake console functions here, so I think it would be reasonable to also define an empty window object too, and evaluate that script before trying to evaluate JSHint instead of after.

@ralfstx
Copy link
Member

ralfstx commented May 26, 2013

Published 0.9.7 which includes the window shim and the latest jshint version, 2.1.2. Performance is not as good as it was in r12, but I think it's acceptable.

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

No branches or pull requests

3 participants