search

magnars / optimus

A Ring middleware for frontend performance optimization.
364 stars 23 forks source link

Use JSR223 to execute JS optimizations w/ GraalJS, Nashorn or Rhino #66

Closed radhikalism closed 4 years ago

radhikalism commented 4 years ago

Summary

Picking up where #52 left off, but with some twists:

Unlike past experience in #52 it seems like all tests now pass (including the dodgy multiple selector test with clean-css), at least so far with a recent JDK (v11).

The PR is open for review but obviously incomplete. What do you think of this approach at a high level?

How to try it

The choice of JS engine is given through environ, so it can be passed like this too:

OPTIMUS_JS_ENGINES=nashorn lein midje (so long as the engine dependency is also on the classpath)

The environ ergonomics is how Optimus users can override the choice of JS engine, alongside declaring necessary engine dependencies and/or exclusions.

Notes

[1] clj-v8 is super outdated and the release situation is questionable. I investigated eclipsesource/j2v8 as an alternative, but they are pivoting to Android-only. If someone makes a fresh v8 JSR223 binding for Java, it may be worth testing against too.

[2] Meanwhile GraalJS looks pretty complete, maintained and performs very reasonably, so it could be a fine default.

[3] Nashorn is deprecated and spits a warning when used in recent JDKs, it may soon be removed entirely.

[4] Rhino doesn't have a JSR223 interface upstream but there is a third-party adapter that seems adequate.

magnars commented 4 years ago

Outstanding! This looks like just what is needed. Thank you for doing this work. What would you say remains to be done?

man. 14. okt. 2019 kl. 20:47 skrev Radhika Reddy notifications@github.com:

Summary

Picking up where #52 https://github.com/magnars/optimus/pull/52 left off, but with some twists:

  • Drop clj-v8 entirely [1]
  • Prefer GraalJS by default (opt-out) [2]
  • Compatible with Nashorn (opt-in, where available for now) [3]
  • Compatible with Rhino (opt-in, where provided and selected) [4]
  • Compatible with conceivably any future JSR223 JS engine loaded by end-users and selected using environ

Unlike past experience in #52 https://github.com/magnars/optimus/pull/52 it seems like all tests now pass (including the dodgy multiple selector test with clean-css), at least so far with a recent JDK (v11).

The PR is open for review but obviously incomplete. What do you think of this approach at a high level? How to try it

  • Checkout
  • lein midje to execute tests with the default JS engine (GraalJS)
  • lein with-profile +rhino midje to execute tests with Rhino
  • lein with-profile +nashorn midje to execute tests with Nashorn extremely slowly (on a suitably old JDK)

The choice of JS engine is given through environ, so it can be passed like this too:

OPTIMUS_JS_ENGINES=nashorn lein midje (so long as the engine dependency is also on the classpath)

The environ ergonomics is how Optimus users can override the choice of JS engine, alongside declaring necessary engine dependencies and/or exclusions. Notes

[1] clj-v8 is super outdated and the release situation is questionable. I investigated eclipsesource/j2v8 https://github.com/eclipsesource/J2V8/ as an alternative, but they are pivoting to Android-only. If someone makes a fresh v8 JSR223 binding for Java, it may be worth testing against too.

[2] Meanwhile GraalJS looks pretty complete, maintained and performs very reasonably, so it could be a fine default.

[3] Nashorn is deprecated and spits a warning when used in recent JDKs, it may soon be removed entirely.

[4] Rhino doesn't have a JSR223 interface upstream but there is a third-party adapter https://bitbucket.org/bunkenburg/rhino-js-engine that seems adequate.

You can view, comment on, or merge this pull request online at:

https://github.com/magnars/optimus/pull/66 Commit Summary

  • Use JSR223 to execute JS optimizations w/ GraalJS, Nashorn or Rhino

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/magnars/optimus/pull/66?email_source=notifications&email_token=AACA4OIWTMYDTGWMPEXNKUTQOS5EDA5CNFSM4JASXM7KYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HRVM57Q, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACA4OKVXEI2XU22FFZZFB3QOS5EDANCNFSM4JASXM7A .

radhikalism commented 4 years ago

What would you say remains to be done?

Not much, at least:

radhikalism commented 4 years ago

@magnars I'd say the bulk of the work is now done, so please feel free to review, comment, or merge as you see fit.

I gave it a spin with local installation and a Compojure site as a single manual integration test. No correctness problems there. GraalJS runs pretty fast for me; Rhino is fine too; Nashorn… well, it completes.

There are also some automated Midje tests for the new optimus.js namespace. I'll look into configuring Travis to execute with non-default JS engines as well, but that work could follow after this PR.

Do you do release candidates? Although this is not theoretically a breaking change for typical Optimus users and the public API, I'd feel more comfortable with some kind of public test period exposing the new choice of JS runtimes to the wild for feedback.

magnars commented 4 years ago

I have now read through all the code, and everything looks good to me. Well done indeed. Let's do a release candidate for this change, and I will start using it in my projects to give it a thorough spin.

magnars commented 4 years ago

Can we get rid of this warning somehow?

lein midje
Warning: implicit hook found: lein-environ.plugin/hooks
Hooks are deprecated and will be removed in a future version.
nil
All checks (1950) succeeded.
magnars commented 4 years ago

I'm not sure if this is something we can fix?

lein with-profile +nashorn midje
Warning: implicit hook found: lein-environ.plugin/hooks
Hooks are deprecated and will be removed in a future version.

FAIL It correctly minifies several rules for the same selector at (minify_test.clj:151)
Expected:
"div,table{border:0}table{margin:0}"
Actual:
"div,table{border:0}"
Diffs: strings have 1 difference (55% similarity)
                expected: "...der:0}(table{margin:0})"
                actual:   "...der:0}(---------------)"
nil
FAILURE: 1 check failed.  (But 1949 succeeded.)

This is with JDK8. The same does not happen on JDK11. I think we'll let this slide.

magnars commented 4 years ago

I have pushed [optimus "1.0.0-rc1"] to clojars. Let's give it a spin. @augustl could you give this a spin on windows? :)

radhikalism commented 4 years ago

Can we get rid of this warning somehow?


lein midje
Warning: implicit hook found: lein-environ.plugin/hooks
Hooks are deprecated and will be removed in a future version.

It looks like this issue is open upstream at weavejester/environ/issues/88 and will have to be fixed there before Leiningen 3.0 removes hooks altogether.

I'm not sure if this is something we can fix? This is with JDK8. The same does not happen on JDK11. I think we'll let this slide.

Yes, I thought it would be low priority too, considering Nashorn will be removed from JDKs eventually. But I have a todo task to look at polyfilling Array.splice to see if it can be fixed trivially (as it was in #52 — but this time the polyfill will need to be applied conditionally).

radhikalism commented 4 years ago

FAIL It correctly minifies several rules for the same selector at (minify_test.clj:151) Expected: "div,table{border:0}table{margin:0}" Actual: "div,table{border:0}" Diffs: strings have 1 difference (55% similarity) expected: "...der:0}(table{margin:0})" actual: "...der:0}(---------------)" nil FAILURE: 1 check failed. (But 1949 succeeded.)



This is with JDK8. The same does not happen on JDK11. I think we'll let this slide.

Added a commit fixing this by patching Array.splice conditionally — only when Nashorn is instantiated on Java 8.

magnars commented 4 years ago

Excellent 👌

lør. 19. okt. 2019 kl. 15:37 skrev radhika reddy notifications@github.com:

FAIL It correctly minifies several rules for the same selector at (minify_test.clj:151) Expected: "div,table{border:0}table{margin:0}" Actual: "div,table{border:0}" Diffs: strings have 1 difference (55% similarity) expected: "...der:0}(table{margin:0})" actual: "...der:0}(---------------)" nil FAILURE: 1 check failed. (But 1949 succeeded.)

This is with JDK8. The same does not happen on JDK11. I think we'll let this slide.

Added a commit fixing this by patching Array.splice conditionally — only when Nashorn is instantiated on Java 8.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/magnars/optimus/pull/66?email_source=notifications&email_token=AACA4ONFJPI5FBZJQNWMUEDQPMETPA5CNFSM4JASXM7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBXP2NA#issuecomment-544144692, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACA4OM3KHPLHG2ACY7GT63QPMETPANCNFSM4JASXM7A .

augustl commented 4 years ago

@augustl could you give this a spin on windows? :)

Got this warning when starting it up:

...
Retrieving com/ibm/icu/icu4j/62.1/icu4j-62.1.jar from central
Retrieving org/graalvm/js/js-scriptengine/19.2.0.1/js-scriptengine-19.2.0.1.jar from central
Retrieving net/java/dev/jna/jna/3.2.2/jna-3.2.2.jar from central
Retrieving optimus/optimus/1.0.0-rc1/optimus-1.0.0-rc1.jar from clojars
Warning: clj-v8-native 0.1.4 will still have its native content in :native-path
  Consider doing `lein clean` to remove potentially stale native files

Did a lein clean, did not try starting up without it.

But 🥁 🥁 🥁 everything works fine on Win10 🎉 🎉 🎉 🎉 🎉

Or, the app starts up fine and some of the web pages I tested works just fine. Will report back with any oddities if there are any.

magnars commented 4 years ago

@augustl That's great to hear! 👏 I can confirm that the warning goes away after a clean. It's just residue from the old clj-v8.

magnars commented 4 years ago

I am planning on working on Optimus all day Thursday in 6 days, mainly focused on merging this and porting the other Optimus packages. Any experiences from testing this I should know about, @radhikalism ?

radhikalism commented 4 years ago

I am planning on working on Optimus all day Thursday in 6 days, mainly focused on merging this and porting the other Optimus packages. Any experiences from testing this I should know about, @radhikalism ?

Nothing to report. It would be good if anyone else has tested the RCs with varying configurations and especially combining optimus-* asset loaders etc. My (current) use case is almost trivial (vanilla Optimus) and it's a new site so regressions are harder to spot.

kendagriff commented 3 years ago

Thanks for all this hard work.

Question: Is there any way for the 0.20.2 code to not rely on clj-v8? On the new Apple M1, the following throws an error on launch:

(defn- find-file-path-fragments
   []
   (let [os-name (System/getProperty "os.name")
         os-arch (System/getProperty "os.arch")]
     (case [os-name os-arch]
       ["Mac OS X" "x86_64"] ["macosx/x86_64/" ".dylib"]
       ["Linux" "x86_64"]    ["linux/x86_64/" ".so"]
       ["Linux" "amd64"]     ["linux/x86_64/" ".so"]
       ["Linux" "x86"]       ["linux/x86/" ".so"]
       ["Linux" "i386"]      ["linux/x86/" ".so"]
       ["Linux" "i486"]      ["linux/x86/" ".so"]
       ["Linux" "i586"]      ["linux/x86/" ".so"]
       ["Linux" "i686"]      ["linux/x86/" ".so"]
       (throw (Exception. (str "Unsupported OS/archetype: " os-name " " os-arch))))))

Even if we use an alternate JS engine, clj-v8 is still being required, and throwing the error. Any suggestions?

magnars commented 3 years ago

It seems like I have a "work in progress" to remove the clj-v8 dependency that I haven't looked at since the end 2019. 😬 I currently have no idea why I didn't finish that work. I'll see if I can figure out what is going on.

kendagriff commented 3 years ago

Thank you thank you! In the meantime, I'm testing RC3 to see if it works.

kendagriff commented 3 years ago

FYI: RC3 fixed this issue with the Apple M1. We've been testing it in our staging environment successfully so far; we'll let you know if we see anything weird.

magnars commented 3 years ago

Excellent, thank you!

tor. 11. mar. 2021 kl. 19:49 skrev Kendall Buchanan < @.***>:

FYI: RC3 fixed this issue with the Apple M1. We've been testing it in our staging environment successfully so far; we'll let you know if we see anything weird.

— You are receiving this because you modified the open/close state.

Reply to this email directly, view it on GitHub https://github.com/magnars/optimus/pull/66#issuecomment-796961975, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACA4OMBBSQ6RATMRFEI2O3TDD7EHANCNFSM4JASXM7A .

augustl commented 3 years ago

IIRC the reason RC3 isn't released is that it requires a lot of plugins to Optimus to also be updated? Since they call directly into clj-v8 stuff in optimus core.

On 11 Mar 2021, at 20:20, Magnar Sveen @.***> wrote:

 Excellent, thank you!

tor. 11. mar. 2021 kl. 19:49 skrev Kendall Buchanan < @.***>:

FYI: RC3 fixed this issue with the Apple M1. We've been testing it in our staging environment successfully so far; we'll let you know if we see anything weird.

— You are receiving this because you modified the open/close state.

Reply to this email directly, view it on GitHub https://github.com/magnars/optimus/pull/66#issuecomment-796961975, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACA4OMBBSQ6RATMRFEI2O3TDD7EHANCNFSM4JASXM7A .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

magnars commented 3 years ago

Aha, yes, excellent recollection. That’s why! Thanks. :-)

tor. 11. mar. 2021 kl. 20:33 skrev August Lilleaas @.***

:

IIRC the reason RC3 isn't released is that it requires a lot of plugins to Optimus to also be updated? Since they call directly into clj-v8 stuff in optimus core.

On 11 Mar 2021, at 20:20, Magnar Sveen @.***> wrote:

 Excellent, thank you!

tor. 11. mar. 2021 kl. 19:49 skrev Kendall Buchanan < @.***>:

FYI: RC3 fixed this issue with the Apple M1. We've been testing it in our staging environment successfully so far; we'll let you know if we see anything weird.

— You are receiving this because you modified the open/close state.

Reply to this email directly, view it on GitHub https://github.com/magnars/optimus/pull/66#issuecomment-796961975, or unsubscribe < https://github.com/notifications/unsubscribe-auth/AACA4OMBBSQ6RATMRFEI2O3TDD7EHANCNFSM4JASXM7A

.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

— You are receiving this because you modified the open/close state.

Reply to this email directly, view it on GitHub https://github.com/magnars/optimus/pull/66#issuecomment-796991402, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACA4ONAERJOTFM6RKAC4ZDTDELILANCNFSM4JASXM7A .

kendagriff commented 3 years ago

In other words, inasmuch as we're not using Optimus plugins, we're free and clear of clj-v8?

magnars commented 3 years ago

Sounds about right.

tor. 11. mar. 2021 kl. 20:47 skrev Kendall Buchanan < @.***>:

In other words, inasmuch as we're not using Optimus plugins, we're free and clear of clj-v8?

— You are receiving this because you modified the open/close state.

Reply to this email directly, view it on GitHub https://github.com/magnars/optimus/pull/66#issuecomment-797002626, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACA4OOU7AU653EEXGX2LQDTDEF3BANCNFSM4JASXM7A .