Closed radhikalism closed 4 years ago
This is some impressive work and a major milestone for Optimus. Thanks for all your efforts, @arbscht! I'll sit down and read through + test this tonight, and hopefully we can look at a 1.0 release in not too long.
"context" is a confusingly overloaded term, so I started using "engine" for one sense in the code. Maybe there a better and more consistent terminology.
Agreed. Context is too vague. Engine is a fine choice.
Is environ a good way to detect engine choice, considering how Optimus is otherwise configured?
What is your thinking on this?
There is no fallback configuration engine, only a default. (I don't think there should be a fallback if the user names an invalid engine…)
Agreed on the no-fallback principle. Invalid choices should lead to exceptions, not unexpected behaviors.
The code looks good to me. Nashorn is pretty slow compared to clj-v8 tho:
v8:
./build-prod.sh
Building app for production
Building web-prod-build ... Done in 2617 ms
Building ios-prod-build ... Done in 2047 ms
Building android-prod-build ... Done in 2134 ms
Nashorn:
OPTIMUS_JS_ENGINE=nashorn ./build-prod.sh
Building app for production
Building web-prod-build ... Done in 44697 ms
Building ios-prod-build ... Done in 45071 ms
Building android-prod-build ... Done in 66985 ms
Hmm, it actually turns out that the CSS-files minified via Nashorn differ from those created with V8. There are some rules missing in the Nashorn version. I'll have to investigate that.
Nashorn and clj-v8 behaves differently like this:
(defn create-clean-css-context
"Minify CSS with the bundled clean-css version"
[engine-name]
(let [engine (js/get-engine engine-name)]
(.eval engine "var window = { XMLHttpRequest: {} };")
(.eval engine clean-css)
engine))
(let [reset {:path "/reset.css"
:contents "table,div {border:0} table {margin:0}"}]
(= (minify-css-asset
(create-clean-css-context "nashorn")
reset {})
(minify-css-asset
(create-clean-css-context "clj-v8")
reset {})))
clj-v8 (correctly) minifies it to:
div,table{border:0}table{margin:0}
while Nashorn minifies it to:
div,table{border:0}
I guess it has nothing to do with this pull request, but it does mean that you can't really trust your CSS to the Nashorn-version.
Any ideas?
Here's a test to reproduce the issue:
(fact
"It correctly minifies several rules for the same selector, on both nashorn and
clj-v8."
(minify-css (create-clean-css-context "clj-v8" ) "table,div {border:0} table {margin:0}" {}) => "div,table{border:0}table{margin:0}"
(minify-css (create-clean-css-context "nashorn") "table,div {border:0} table {margin:0}" {}) => "div,table{border:0}table{margin:0}")
It requires this modified code:
(defn create-clean-css-context
"Minify CSS with the bundled clean-css version"
([] (create-clean-css-context nil))
([engine-name]
(let [engine (if engine-name
(js/get-engine engine-name)
(js/get-engine))]
(.eval engine "var window = { XMLHttpRequest: {} };")
(.eval engine clean-css)
engine)))
I have tried updating to the latest clean-css (3.4.12), but that didn't help any.
Hm, the clean-css+nashorn situation is concerning. I looked into it a bit and found that running a test script in JS via Avatar JS CLI (using Nashorn) works the same as node/V8 CLI, but Nashorn via Clojure REPL does not. I may be missing some initialization config or ceremony, which perhaps clean-css assumes about its environment. I'll investigate more.
Performance in Nashorn is slower in general I suspect because we're crudely isolating code using fresh engine instances instead of JSR-223's ScriptContexts/scoping on a singleton engine. This is because clj-jsr223-v8 doesn't yet support that part of the API (yet happens to be fast because clj-v8 does a similar thing), so uniform client code can't be written. Wiring it up is on the todo list for clj-jsr223-v8, after which we can run a fair comparison. (That may also obviate the context/engine terminology mismatch.)
Regarding environ, it may be fine to use for this one setting. I just haven't looked into how it fits with existing configuration options and whether that's inconsistent, or if environ would be a better configuration story for any of them too.
Thanks for reviewing.
Well, what do you know. Nashorn implements Array.splice
differently (correctly) compared to pretty much everyone else, including v8 and spidermonkey. CleanCSS assumes that splice behaves the typical but non-standard way deep in a code path under advanced optimizations.
Meanwhile, Avatar JS takes the approach of monkey-patching Array.prototype.splice
for compatibility. I've updated ns optimus.js to do something similar with a generic init-engine
multimethod.
I think there are not likely to be very many of these issues, so I'd continue to have confidence in Nashorn.
The suspect splice call is at clean-css.js:10467
:
while (tokenIdx >= 0) {
if ((tokenIdx === 0 || (optimizedBody.tokenized[propertyIdx] && bodiesAsList[tokenIdx].indexOf(optimizedBody.tokenized[propertyIdx].value) > -1)) && propertyIdx > -1) {
propertyIdx--;
continue;
}
var newBody = {
list: optimizedBody.list.splice(propertyIdx + 1),
tokenized: optimizedBody.tokenized.splice(propertyIdx + 1)
};
options.callback(tokens[processedTokens[tokenIdx]], newBody, processedCount, tokenIdx);
tokenIdx--;
}
So I added a new init-engine
hook which executes this in every Nashorn engine:
(function(){
var nashorn_splice = Array.prototype.splice;
Array.prototype.splice = function() {
if (arguments.length === 1) {
return nashorn_splice.call(this, arguments[0], this.length);
} else {
return nashorn_splice.apply(this, arguments);
}}})();
Another thing to consider: how should Optimus depend on JDK8+ for testing? Unit tests targeting "nashorn" won't work in older JDKs (as currently configured in Travis). We may need platform-specific test profiles or guards.
Impressive detective work. Well done!
Another thing to consider: how should Optimus depend on JDK8+ for testing?
optimus.homeless
has a (jdk-version)
that can be used to exclude nashorn-specific tests on a per-test basis. I think that sounds like a reasonable solution.
I'm wary of using (jdk-version)
strings and conditionals (code smell?) — unless I can find a clean way of doing so.
Here's another possibility, fully specifying the Travis build matrix:
language: clojure
lein: lein2
script: lein2 midje
matrix:
include:
- jdk: openjdk6
- jdk: openjdk7
- jdk: oraclejdk7
- jdk: oraclejdk8
- jdk: oraclejdk8
env: OPTIMUS_JS_ENGINE=nashorn
Outside of Travis, developers would have to select their JS engine locally as needed, rather than always running all combinations of available engines.
Does that seem reasonable? If not, I'll dig into guarding unit tests based on versions.
If we have the ability to do runtime checks to determine the presence of Nashorn, my suggestion is that we simply check for it around the tests. man. 2. mai 2016 kl. 19.02 skrev Abhishek Reddy notifications@github.com:
I'm wary of using (jdk-version) strings and conditionals (code smell?) — unless I can find a clean way of doing so.
Here's another possibility I started exploring:
- The value of :optimus-js-engine from environ can be a comma-separated list of engines in order of decreasing preference. e.g. OPTIMUS_JS_ENGINE=nashorn,clj-v8 will try nashorn first, then clj-v8.
- if no engine in the list can be found, an exception is thrown.
- Travis can generate a build matrix including oraclejdk8, and two forms of :optimus-js-engine:
jdk:
- openjdk6
- openjdk7
- oraclejdk7
- oraclejdk8env:
- OPTIMUS_JS_ENGINE=clj-v8,nashorn
- OPTIMUS_JS_ENGINE=nashorn,clj-v8
That should try eight different builds:
- OPTIMUS_JS_ENGINE=clj-v8,nashorn openjdk6 # Use clj-v8- OPTIMUS_JS_ENGINE=clj-v8,nashorn openjdk7 # Use clj-v8- OPTIMUS_JS_ENGINE=clj-v8,nashorn oraclejdk7 # Use clj-v8- OPTIMUS_JS_ENGINE=clj-v8,nashorn oraclejdk8 # Use clj-v8- OPTIMUS_JS_ENGINE=nashorn,clj-v8 openjdk6 # Fallback to clj-v8, redundant- OPTIMUS_JS_ENGINE=nashorn,clj-v8 openjdk7 # Fallback to clj-v8, redundant- OPTIMUS_JS_ENGINE=nashorn,clj-v8 oraclejdk7 # Fallback to clj-v8, redundant- OPTIMUS_JS_ENGINE=nashorn,clj-v8 oraclejdk8 # Use nashorn
The redundant jobs are unfortunate but I can't see any way to be more specific with Travis.
Does that seem reasonable? If not, I'll dig into guarding unit tests based on versions.
— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/magnars/optimus/pull/52#issuecomment-216293180
Runtime checking is doable.
One way is to wrap platform-specific test bodies in conditionals checking against (jdk-version)
. I wasn't keen on this for two reasons.
My first instinct was to use Midje's filter-by-metadata capability. That allows tagging facts for, say, a specific platform. But I can't see how to apply the filter without manual intervention anyway. Likewise for switching on/off certain test paths etc etc.
Anyway, here's a potential Nashorn-specific fact check at runtime:
(when (optimus.js/engine-exists? "nashorn")
(fact
"It correctly minifies several rules for the same selector."
(minify-css (create-clean-css-context (optimus.js/get-engine "nashorn")) "table,div {border:0} table {margin:0}" {}) => "div,table{border:0}table{margin:0}"))
Acceptable?
That looks good to me.
Actually, I've just learned how to configure Midje to programmatically filter facts tagged with metadata. I think this is better:
(fact
:nashorn-only
"It correctly minifies several rules for the same selector."
(minify-css (create-clean-css-context (optimus.js/get-engine "nashorn")) "table,div {border:0} table {margin:0}" {}) => "div,table{border:0}table{margin:0}"))
.midge.clj
:
(require 'optimus.js)
;; Exclude :nashorn-only tests if we can't detect nashorn in this JDK
(when-not (optimus.js/engine-exists? "nashorn")
(change-defaults :fact-filter (complement :nashorn-only)))
I need to refactor some code and tidy up docs as well. Updates to come.
Hi, Any progress on this PR? Unfortunately I'm developing on Windows! Please! :D
Closing in favor of #66.
Changes:
optimus.js
talks tojavax.script.*
API to get/control script engines.optimus.optimizations.minify
usesoptimus.js
to create contexts and evaluate scripts.:optimus-js-engine
setting via environ.profiles.clj
, shell environment, or anywhere else as documented.OPTIMUS_JS_ENGINE=nashorn lein midje
will run tests using Nashorn (assuming JDK8).Testing:
optimus.js-test
.openjdk version "1.8.0_77-Debian"
,lein midje
reports "All checks (1908) succeeded." for either "clj-v8" or "nashorn" as:optimus-js-engine
.Some other aspects to review:
eval(…)
, not directly as before.JSON.stringify
in JS andclojure.data.json/read-str in Clojure
).cleanup-engine
is optional if relying onfinalize()
is acceptable (but I've left it in anyway for now).