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

Update or replace therubyracer #2100

Closed
mkllnk opened this issue Feb 28, 2018 · 15 comments · Fixed by #4287
Closed

Update or replace therubyracer #2100

mkllnk opened this issue Feb 28, 2018 · 15 comments · Fixed by #4287
Assignees

Comments

@mkllnk
Copy link
Member

mkllnk commented Feb 28, 2018

Description

We use the gem therubyracer 0.12.0 which is five years old. It embeds a very old version of the V8 Javascript engine. V8 had a lot of improvements done in these five years and a newer version could make our asset compilation faster and more secure. The Discourse people decided to replace it with the mini_racer and their benchmarks show that it has been a good decicion. https://github.com/discourse/mini_racer#benchmark-minification-of-discourse-applicationjs-both-minified-and-unminified

Expected Behavior

Asset compilation is fast and succeeds.

Actual Behavior

Asset compilation is slow and sometimes takes more memory than the server has.

Steps to Reproduce

  1. Run: bundle exec rake RAILS_ENV=staging assets:precompile:primary RAILS_GROUPS=assets

Context

Someone contacted us via email about using the OFN as backend for an app. Their developer saw the outdated gem therubyracer as potential issue for security and for installing the OFN on a BSD operating system. Looking into that feedback I found that we would probably benefit from updating or replacing that gem anyway, even though the current community is not interested in using a BSD instead of Linux as operating system.

Severity

Sometimes asset compilation can crash which requires a developer to run it manually again. Long asset compilation can cause short disruptions to users while deploying a new release, but usually we manage to do it without downtime. So I guess it's severity 5.

Possible Fix

Update therubyracer or replace it with mini_racer.

@mkllnk mkllnk added the bug-s5 We can live with it... Few users are impacted. label Feb 28, 2018
@kirstenalarsen
Copy link
Contributor

@mkllnk @myriamboure @enricostano @sauloperez @daniellemoorhead is this a bug or part of tech debt / upgrade dependencies? suspect if classified as bug-s5 if will never get done, but if we have way to track in upgrades it may be more likely to get picked up?

@mkllnk
Copy link
Member Author

mkllnk commented Feb 28, 2018

I would classify it as tech debt, but I don't know how to do that with the new issue structure. Do we have a process or template for that?

@enricostano
Copy link
Contributor

Yep, tech debt. Not bug. Should be an icebox, right?

I'm still not sure how to procede about this specific kind of tasks, any idea @daniellemoorhead ?

@myriamboure
Copy link
Contributor

Yes, I would create an icebox item (a bit same style that of those you created with Pau @enricostano and need to review, I added them on the spreadsheet as well but was not sure how to describe the focus). This for instance could be one of the feature candidate to a focus like "Make deployment easier, more secure and smooth". If that sounds right please @mkllnk or @enricostano create an icebox item on that focus point and close this bug (put the link in the icebox to keep track) :-)

@myriamboure
Copy link
Contributor

We will then decide to prioritize that in our next or further product curation priorization session.

@daniellemoorhead daniellemoorhead added tech debt and removed bug-s5 We can live with it... Few users are impacted. labels Mar 2, 2018
@daniellemoorhead
Copy link
Contributor

Created a label called 'tech debt' so that we don't lose these small kinds of things.

Also, should we maybe have a steady stream of tech debt small tweaks like this factored into our pipe, just like we do for sys admin stuff @kirstenalarsen @myriamboure @enricostano?

@myriamboure
Copy link
Contributor

I don't know if those are just small tweaks, I guess we could...but hard to judge for me is small or not.

@mkllnk
Copy link
Member Author

mkllnk commented Mar 6, 2018

I would think that this is done in one hour. If not, something unexpected happened and we need to review this. So I would timebox it to one or two hours.

@myriamboure
Copy link
Contributor

Given the performance and deployment issues we have I would suggest to prioritize it and timebox to max 2h. If any objection @enricostano raise your voice :-) Can you put that on your list @mkllnk or do we need to find someone else to work on it?

@myriamboure
Copy link
Contributor

Put it in delivery train backlog as not sure it require more inception, if not please feel free to move directly to dev ready.

@enricostano
Copy link
Contributor

enricostano commented Mar 7, 2018

2hrs as a first try it's OK. We'll see when we'll be able to actually work on it though.

EDIT: I mean that we have more urgent stuff in the pipeline IMHO

@myriamboure
Copy link
Contributor

We decided in product curation meeting that there were other more urgent priority, so didn't prioritized, putting that back to "all the things" and when things move forward can be addressed later.

@mkllnk mkllnk removed their assignment Jun 22, 2018
@frank-west-iii
Copy link
Collaborator

We are currently on therubyracer v0.12.0 and the most up to date version is v0.12.3.

I ran the command with each version that we are considering and here are my non-scientific results.

The change to mini_racer was simple enough without any problems during bundling. If you would like I can PR this change.

The Ruby Racer 0.12.0

time bundle exec rake RAILS_ENV=staging assets:precompile:primary RAILS_GROUPS=assets
bundle exec rake RAILS_ENV=staging assets:precompile:primary   316.42s user 10.74s system 99% cpu 5:27.72 total

The Ruby Racer 0.12.3

time bundle exec rake RAILS_ENV=staging assets:precompile:primary RAILS_GROUPS=assets
bundle exec rake RAILS_ENV=staging assets:precompile:primary   351.18s user 12.63s system 99% cpu 6:05.50 total

Mini Racer

time bundle exec rake RAILS_ENV=staging assets:precompile:primary RAILS_GROUPS=assets
bundle exec rake RAILS_ENV=staging assets:precompile:primary   211.43s user 28.24s system 102% cpu 3:53.99 total

@mkllnk
Copy link
Member Author

mkllnk commented Jul 2, 2018

Cutting down asset compile time by a third sounds nice. :-) Go for it!

@Matt-Yorkley
Copy link
Contributor

See: #3244

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

Successfully merging a pull request may close this issue.

9 participants