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

Use libv8 node #186

Merged
merged 10 commits into from
Apr 7, 2021
Merged

Use libv8 node #186

merged 10 commits into from
Apr 7, 2021

Conversation

lloeki
Copy link
Collaborator

@lloeki lloeki commented Jan 18, 2021

This is a PR for people to try mini_racer using libv8-node built for Apple M1 or musl.

Note: this is only tested using a rbenv install'd Ruby (which builds as arm64), and untested using the system-provided Ruby (which is built as arm64e) as I have not yet found a way to build libv8/node as arm64e.

Since I don't fully consider libv8-node ready for prime time yet, the original intent is not to merge this as is (if ever), but provide an easy testing ground via:

# Gemfile
gem 'mini_racer', github: 'rubyjs/mini_racer', branch: 'refs/pull/186/head'
# or
gem 'mini_racer', github: 'sqreen/mini_racer', branch: 'use-libv8-node'

davidtaylorhq added a commit to discourse/discourse that referenced this pull request Jan 27, 2021
After this, the only remaining issue preventing Discourse from booting on apple silicon is mini_racer/libv8. See upstream discussion at rubyjs/mini_racer#186 for an experimental solution.
davidtaylorhq added a commit to discourse/discourse that referenced this pull request Jan 27, 2021
After this, the only remaining issue preventing Discourse from booting on apple silicon is mini_racer/libv8. See upstream discussion at rubyjs/mini_racer#186 for an experimental solution.
@sweetppro
Copy link

sweetppro commented Jan 27, 2021

I get this error when running bundle install on my M1 MacBook Air...

Could not find gem 'libv8-node (~> 15.5.1.0.beta1)', which is required by gem 'mini_racer', in any
of the sources.

@lloeki
Copy link
Collaborator Author

lloeki commented Jan 27, 2021

@sweetppro Might be the same as this one:

rubyjs/libv8-node#14 (comment)

@SamSaffron
Copy link
Collaborator

@lloeki we now have 3 developers that had lots of luck with the the new gem. @davidtaylorhq was amongst them.

Performance on Discourse is pretty much spectacular on native M1. Looking forward to getting this new pattern into a production gem!

@lloeki
Copy link
Collaborator Author

lloeki commented Jan 28, 2021

I get this error when running bundle install on my M1 MacBook Air...

Sharing this finding here as well, one would really want to use Ruby >= 2.7.2 on M1, as versions before that are more or less subtly broken.

@2called-chaos
Copy link

There is no way of utilizing Bundler's (2.2) enhanced platform support to auto-switch? I mean in the Gemfile directly, I guess using a local version via bundler config local.mini_racer /some/path is a way but requires some setup steps for M1 users.

@lloeki
Copy link
Collaborator Author

lloeki commented Apr 6, 2021

GitHub Actions results for 6b38556 here and here.

It fails on aarch64-linux (except on Ruby 3.0, go figure), might be because of the build on Apple Silicon (through a VM), might be because of QEMU, or an interaction between both:

*** stack smashing detected ***: <unknown> terminated
qemu: uncaught target signal 6 (Aborted) - core dumped
Aborted (core dumped)

Travis seems to be fine with the build, although agonisingly slow to update rubygems.

Ruby 3.0 aarch64-linux-musl fails but that might be because there's some specifics to that Docker image (I noticed the build env seems weirdly broken by default, e.g CXX='')

@SamSaffron
Copy link
Collaborator

Do you feel we are in a good enough state to start gathering public feedback by doing a pre-release gem of mini_racer?

@lloeki
Copy link
Collaborator Author

lloeki commented Apr 7, 2021

Yeah, I think it'll solve more issues than it "creates" (on platforms that weren't there before).

@lloeki lloeki merged commit 992c49e into rubyjs:master Apr 7, 2021
@lloeki
Copy link
Collaborator Author

lloeki commented Apr 7, 2021

BTW I think we can go straight to 0.4.0 (.beta1) given the change's magnitude.

@SamSaffron
Copy link
Collaborator

SamSaffron commented Apr 7, 2021 via email

@SamSaffron
Copy link
Collaborator

SamSaffron commented Apr 8, 2021

@lloeki I just pushed mini_racer 0.4.0.beta1 using node libv8! 🎊

Asking some colleagues with m1 macs to test it out! Seems to work fine on my Linux install.

@tisba
Copy link
Collaborator

tisba commented Apr 8, 2021

Happy do run some tests on m1. Do you want to get feedback/issues here?

@sweetppro
Copy link

It seems to run just fine on my m1 MacBook Air.
Rails 6.1.3.1
Ruby 2.7.2p137
rbenv 1.1.2

@lloeki
Copy link
Collaborator Author

lloeki commented Apr 8, 2021

Thanks @SamSaffron!

@tisba positive reports should be OK here in this discussion, but please create a dedicated issue if there's a problem.

@jdelStrother
Copy link

seems good! At least from a casual test:

irb(main):003:0> context = MiniRacer::Context.new
=> #<MiniRacer::Context:0x000000010f8faad8 @functions={}, @timeout=nil, @max_memory=nil, @current_exception=nil, @isolate=false, @ensure_gc_after_idle=nil, @disposed=false, @ca...
irb(main):004:0> context.eval 'var adder = (a,b)=>a+b;'
=> nil
irb(main):005:0> puts context.eval 'adder(20,22)'
42
=> nil
irb(main):006:0> RUBY_PLATFORM
=> "arm64-darwin20"
irb(main):007:0> RUBY_VERSION
=> "2.7.2"
irb(main):008:0> MiniRacer::VERSION
=> "0.4.0.beta1"

@SamSaffron
Copy link
Collaborator

@lloeki all our devs are having great success, should we go ahead and promote both v8 and mini_racer to release?

@tisba
Copy link
Collaborator

tisba commented Apr 9, 2021

I have various crashes with the beta under M1 + Docker for Mac. I did not come around yet to compile details :(

@lloeki
Copy link
Collaborator Author

lloeki commented Apr 9, 2021

@tisba That's on aarch64-linux then? Please open a separate issue with the details.

Also can you try installing the ruby platform gem (which will take a while as it builds V8 upon install) and see if you have the same issues?

@lloeki
Copy link
Collaborator Author

lloeki commented Apr 9, 2021

Yup @SamSaffron let's do this. I'll push the updated libv8-node (there's node 15.14.0 out now) and then proceed with mini_racer.

@tisba
Copy link
Collaborator

tisba commented Apr 10, 2021

Using a slight modified version of #170, using rvm 2.7.2,2.7.3,3.0.0,3.0.1 do ruby ./mini_racer_hang.rb, everything looks fine native on my Mac Mini M1.

@tisba
Copy link
Collaborator

tisba commented Apr 14, 2021

Happy to report that the bigger test suite with a lot of mini_racer usage works fine natively on M1 using 0.4.0.beta1! We kind of depend on also using it inside Docker which is not working yet (see #190) but the progress here makes me feel very optimistic. Thanks again for the work! 🙇

@bf4
Copy link

bf4 commented Apr 21, 2021

For me the upgrade (change from libv8 (~> 8.4.255) to libv8-node (~> 15.14.0.0)) broke my usage of String.prototype.localeCompare. I probably need to make an issue in libv8-node but wanted to share here.

function_body = <<~EOS
function localeCompare(a, b) {
  return (a || '').localeCompare((b || ''), 'en', { sensitivity: 'base' });
}
EOS

function = 'localeCompare'
inputs = [
  "Rich",
  "Rich"
]
expectation = 0

snapshot = MiniRacer::Snapshot.new(function_body)
context = MiniRacer::Context.new(snapshot: snapshot)
result = context.call(function, *inputs) # throws RangeError: Internal error. Icu error.
expect(result).to eq(expectation) 

looks like libv8-node is compiled with small-icu which per https://nodejs.org/api/intl.html may let me specify env NODE_ICU_DATA=/some/directory node or something but also says I can check const hasICU = typeof Intl === 'object'; if I need to

In the end used a good-enough polyfill

function localeCompare(a, b) {
  const str = (a || '').toLowerCase();
  const other_str = (b || '').toLowerCase();
  const lang = 'en';
  if (hasWorkingICU(lang)) {
    return str.localeCompare(other_str, lang, { sensitivity: 'base' });
  } else {
    // good-enough polyfill
    const characterMaps =  {
      'en': '� _-,;:!?.\'"()[]{}@*/\&#%`^+<=>|~$0123456789aAbBcCdDeEfFgGhHiIjJkKlLmMnNoOpPqQrRsStTuUvVwWxXyYzZ',
    };
    const map = characterMaps[lang];
    let charA = null;
    let charB = null;
    let index = 0;
    const maxIndex = Math.max(str.length, other_str.length);
    while (charA === charB && index < maxIndex) {
      charA = str[index];
      charB = other_str[index];
      index++;
    }
    return Math.max(-1, Math.min(1, map.indexOf(charA) - map.indexOf(charB)));
  }
}

function hasWorkingICU(lang) {
  const hasICU = typeof Intl === 'object';
  let isMissingLang = false;
  if (hasICU) {
    try {
      'foo'.localeCompare('bar', lang);
    } catch (e) {
      isMissingLang = e.name === 'RangeError';
    }
  }
  return hasICU && !isMissingLang;
}

@SamSaffron
Copy link
Collaborator

SamSaffron commented Apr 22, 2021

@lloeki I guess there may be some more ICU toggles we can use during the compile? I wonder if we should do a full-icu compile? How much bigger will the binary get.

@bf4 can you test in default node installs?

@lloeki
Copy link
Collaborator Author

lloeki commented Apr 22, 2021

It's a configure/build option. I remember dropping to small-icu but can't remember the reason (either size, or a failing build).

I'll try to do some builds with a bigger ICU.

@lloeki
Copy link
Collaborator Author

lloeki commented Apr 27, 2021

@bf4 Builds are out, a mini_racer install or pristine should do the work to build against the new one.

We should probably keep further discussion on that at #196.

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

Successfully merging this pull request may close these issues.

7 participants