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 to libv8 6.0 #65

Closed
seanmakesgames opened this issue Aug 24, 2017 · 65 comments
Closed

update to libv8 6.0 #65

seanmakesgames opened this issue Aug 24, 2017 · 65 comments

Comments

@seanmakesgames
Copy link
Contributor

am running my test suite against 6.0.286.54.0beta1
wanted a place to write down the results. :)
@SamSaffron @ignisf

@seanmakesgames
Copy link
Contributor Author

I'm getting

mkdir -p tmp/x86_64-darwin15/mini_racer_extension/2.2.4
cd tmp/x86_64-darwin15/mini_racer_extension/2.2.4
/Users/user/.rvm/rubies/ruby-2.2.4/bin/ruby -I. ../../../../ext/mini_racer_extension/extconf.rb
checking for main() in -lpthread... yes
checking for main() in -lobjc... yes
creating Makefile
cd -
cd tmp/x86_64-darwin15/mini_racer_extension/2.2.4
/usr/bin/make
compiling ../../../../ext/mini_racer_extension/mini_racer_extension.cc
clang: warning: argument unused during compilation: '-rdynamic'
linking shared-object mini_racer_extension.bundle
clang: warning: libstdc++ is deprecated; move to libc++
ld: unknown option: --start-group
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [mini_racer_extension.bundle] Error 1
rake aborted!
Command failed with status (2): [/usr/bin/make...]
/Users/user/.rvm/gems/ruby-2.2.4/gems/rake-compiler-0.9.9/lib/rake/extensiontask.rb:157:in `block (2 levels) in define_compile_tasks'
/Users/user/.rvm/gems/ruby-2.2.4/gems/rake-compiler-0.9.9/lib/rake/extensiontask.rb:156:in `block in define_compile_tasks'
/Users/user/.rvm/gems/ruby-2.2.4/bin/ruby_executable_hooks:15:in `eval'
/Users/user/.rvm/gems/ruby-2.2.4/bin/ruby_executable_hooks:15:in `<main>'
Tasks: TOP => compile => compile:x86_64-darwin15 => compile:mini_racer_extension:x86_64-darwin15 => copy:mini_racer_extension:x86_64-darwin15:2.2.4 => tmp/x86_64-darwin15/mini_racer_extension/2.2.4/mini_racer_extension.bundle
(See full trace by running task with --trace)

on rake clean compile

is my platform built @ignisf or am I doing something else wrong?

@seanmakesgames
Copy link
Contributor Author

looks like this issue is #66 and I think related to the --start-group linker error (unknown option)
The weird thing is that it's inconsistent on when it shows up. Will resolve on other issue.

@ignisf
Copy link
Collaborator

ignisf commented Aug 24, 2017

rubyjs/libv8@9977436

@seanmakesgames
Copy link
Contributor Author

haha thanks @ignisf -- can't tell if this is introducing the issue or fixing it? Assuming introduces, but don't understand how this is happening on mini_racer mainline (through bundler) when dependency is @ 5.9
Maybe my grasp of what bundler is doing is poor. :\

@ignisf
Copy link
Collaborator

ignisf commented Aug 24, 2017

I'll revert this commit and release another beta. Bundler is not selecting the correct version of libv8 I think. Try uninstalling 6.*. Sorry for taking so long to respond. My ISP has a broken optical connection.

@seanmakesgames
Copy link
Contributor Author

it's all good @ignisf! Thanks for the response.
Good luck with your internet!

@seanmakesgames
Copy link
Contributor Author

You were right-- uninstalling 6.0 got the correct libv8 version to load up, and has isolated the issue to 6.0beta. Thanks for the help @ignisf :)
Awaiting new beta for testing.

@ignisf
Copy link
Collaborator

ignisf commented Aug 25, 2017

6.0 beta2 is up

@seanmakesgames
Copy link
Contributor Author

Everything works @ignisf thanks!
Running test suite now.

@seanmakesgames
Copy link
Contributor Author

Tests all check out. :)

@ignisf
Copy link
Collaborator

ignisf commented Aug 26, 2017

6.0 released 🎉

@krzysiek1507
Copy link

@seanmakesgames are you testing it on production again? :D

@ignisf
Copy link
Collaborator

ignisf commented Aug 29, 2017

That's just how he roll :)

@krzysiek1507
Copy link

krzysiek1507 commented Aug 29, 2017

Can you see significant performance improvement?

@seanmakesgames
Copy link
Contributor Author

Haha, nah-- was running against my test suite. I'll be 'testing on production' once mini_racer has upped its version number. ;)

I didn't do any benchmarking as part of my test cases because I run them multicore. It did 'feel' faster, but that's not useful information. :P

@krzysiek1507
Copy link

Haha. When I updated Node to 8.4.0 (with v8 6.0) it was about 40% faster.
Do you have any ETA? :)

@seanmakesgames
Copy link
Contributor Author

seanmakesgames commented Aug 29, 2017

That's a question for @SamSaffron (whom I'm sure is busy atm, or he'd probably have said something already) ;)

@SamSaffron
Copy link
Collaborator

@krzysiek1507 I don't have an eta, but once @seanmakesgames is happy that 6.0 is stable I am happy to bounce the version.

My initial testing definitely did not show a 40% improvement in my microbench maybe 5-10%

@SamSaffron
Copy link
Collaborator

Note, I am travelling over the next 3 weeks so I am not 100% sure I will be super fast here.

@krzysiek1507
Copy link

Thanks, guys! You are doing great work!

@seanmakesgames
Copy link
Contributor Author

@SamSaffron 6.0 appears stable. I'm good with a rev bump.

@krzysiek1507
Copy link

krzysiek1507 commented Sep 1, 2017

When I installed it from my repo I saw error:

/bundle/bin/rake assets:precompile: symbol lookup error: /bundle/bundler/gems/mini_racer-458c7fafa7a0/lib/mini_racer_extension.so: undefined symbol: _ZN2v84base5debug27EnableInProcessStackDumpingEv

Version: libv8 6.0.286.54.1 (x86_64-linux) on Debian jessie

@krzysiek1507
Copy link

Any update on this?

@SamSaffron
Copy link
Collaborator

Will do another release next week

@krzysiek1507
Copy link

Thanks!

@SamSaffron
Copy link
Collaborator

confirmed @ignisf getting:

/home/sam/.rbenv/versions/2.3.1/bin/ruby: symbol lookup error: /home/sam/Source/mini_racer/lib/mini_racer_extension.so: undefined symbol: _ZN2v84base5debug27EnableInProcessStackDumpingEv

on GCC with 6.0.286 , can you have a look?

@ignisf
Copy link
Collaborator

ignisf commented Sep 25, 2017

morning, back from vacation, will try to check it out

@SamSaffron
Copy link
Collaborator

SamSaffron commented Sep 25, 2017 via email

@ignisf
Copy link
Collaborator

ignisf commented Oct 23, 2017

the missing symbol was due to the reordering of the libraries. Who knew Google's wiki would lie... Should be fixed in master and the 6.0 branches

@ignisf
Copy link
Collaborator

ignisf commented Dec 13, 2017

image

@ignisf
Copy link
Collaborator

ignisf commented Dec 13, 2017

just an FYI, we will be losing FreeBSD support with this version due to lack of upstream support. I decided to not hold up the release more. Will hopefully work out a solution and release binaries at a later stage.

@seanmakesgames
Copy link
Contributor Author

Fine by me. ;)
I'm excited to see how this release performs in the live environment. 😄

@SamSaffron
Copy link
Collaborator

OK, my plan for tomorrow is to bench it on Discourse!

@SamSaffron
Copy link
Collaborator

SamSaffron commented Dec 15, 2017 via email

@SamSaffron
Copy link
Collaborator

SamSaffron commented Dec 15, 2017 via email

@seanmakesgames
Copy link
Contributor Author

That's strange. My full test suite passed. :\

@SamSaffron
Copy link
Collaborator

oops @ignisf looks like 6.2 is hanging for me during script runs. I will try to build a repro for this. I wonder if this was a temporary bug in v8 or if it is fixed in 6.3

6.0 seems to work fine though.

@SamSaffron
Copy link
Collaborator

the repro of the bug is to run: https://github.com/discourse/discourse/blob/master/script/benchmarks/markdown/bench.rb but that involves a full discourse install...

@ignisf
Copy link
Collaborator

ignisf commented Dec 15, 2017

will look into building betas for 6.1 and 6.3

@ignisf
Copy link
Collaborator

ignisf commented Dec 18, 2017

@SamSaffron, just released 6.3 beta.

image

@SamSaffron
Copy link
Collaborator

Oh no :( looks like this is hanging as well, going to create a simple repro script here and ask v8 folks for help.

@ignisf
Copy link
Collaborator

ignisf commented Dec 18, 2017

@SamSaffron, are you sure you're testing with mini_racer linked against the 6.3 beta and not by any chance linked against 6.2?

@SamSaffron
Copy link
Collaborator

yes, 100% 6.3.292.48.0beta1 in my gemfile lock ... trying to build a repro

@SamSaffron
Copy link
Collaborator

SamSaffron commented Dec 18, 2017 via email

@SamSaffron
Copy link
Collaborator

OK, v8 team say it backports very cleanly, can you try maybe adding that patch and doing another 6.3 release?

@ignisf
Copy link
Collaborator

ignisf commented Dec 18, 2017

Hey, sounds good, will look into backporting.

@ignisf
Copy link
Collaborator

ignisf commented Dec 18, 2017

rubyjs/libv8@5777dbd

Will cut another release for you to test tomorrow if the CI passes tonight :)

@SamSaffron
Copy link
Collaborator

SamSaffron commented Dec 19, 2017 via email

@ignisf
Copy link
Collaborator

ignisf commented Dec 19, 2017

image

@seanmakesgames
Copy link
Contributor Author

@SamSaffron let me know when you've tested the patch on this. I'll run my suite against it after.

@SamSaffron
Copy link
Collaborator

SamSaffron commented Dec 20, 2017

Looking good on my side! @ignisf bench is no longer hanging, 6.3 appears slower than 5.9 in some of the bench suite (and faster in other bits):

5.9

tiny post sanitize: true
                        129.364  (±10.8%) i/s -    636.000  in   5.014269s
giant post sanitize: true
                        106.396  (±10.3%) i/s -    530.000  in   5.073153s
most features sanitize: true
                         54.285  (± 7.4%) i/s -    272.000  in   5.039917s
lots of mentions sanitize: true
                          0.404  (± 0.0%) i/s -      3.000  in   7.428951s
tiny post sanitize: false
                        284.413  (± 8.8%) i/s -      1.430k in   5.071733s
giant post sanitize: false
                        207.441  (± 7.2%) i/s -      1.050k in   5.092453s
most features sanitize: false
                         74.881  (± 6.7%) i/s -    378.000  in   5.071991s
lots of mentions sanitize: false
                          0.403  (± 0.0%) i/s -      3.000  in   7.441425s
markdown it no extensions commonmark tiny post
                          7.832k (± 4.3%) i/s -     39.744k in   5.083833s
markdown it no extensions commonmark giant post
                        916.757  (± 9.5%) i/s -      4.550k in   5.054635s
markdown it no extensions commonmark most features
                          1.493k (± 6.3%) i/s -      7.560k in   5.087277s
markdown it no extensions commonmark lots of mentions
                          2.089k (± 5.5%) i/s -     10.556k in   5.070508s

6.3

tiny post sanitize: true
                        129.675  (±10.8%) i/s -    648.000  in   5.070792s
giant post sanitize: true
                        106.214  (±10.4%) i/s -    530.000  in   5.057866s
most features sanitize: true
                         52.109  (±11.5%) i/s -    260.000  in   5.054359s
lots of mentions sanitize: true
                          0.335  (± 0.0%) i/s -      2.000  in   5.988207s
tiny post sanitize: false
                        257.299  (±10.9%) i/s -      1.288k in   5.073169s
giant post sanitize: false
                        208.196  (± 9.6%) i/s -      1.050k in   5.093251s
most features sanitize: false
                         75.199  (± 6.6%) i/s -    378.000  in   5.053536s
lots of mentions sanitize: false
                          0.393  (± 0.0%) i/s -      2.000  in   5.091215s
markdown it no extensions commonmark tiny post
                          7.643k (± 4.3%) i/s -     38.743k in   5.078730s
markdown it no extensions commonmark giant post
                        934.341  (±12.2%) i/s -      4.592k in   5.079925s
markdown it no extensions commonmark most features
                          1.472k (± 3.6%) i/s -      7.452k in   5.068865s
markdown it no extensions commonmark lots of mentions
                          2.054k (± 6.9%) i/s -     10.400k in   5.095029s

Benchmark is: https://github.com/discourse/discourse/blob/master/script/benchmarks/markdown/bench.rb

@ignisf
Copy link
Collaborator

ignisf commented Dec 20, 2017

.0 and .1 releases are up on rubygems.

@seanmakesgames
Copy link
Contributor Author

prepping to run the suite on this (have been making changes, so want to make sure tests are passing first) ;)
Will be running tests against 6.3.292.48.1 and let you know how it goes.

@seanmakesgames
Copy link
Contributor Author

everything looks good on my end. tests pass. no funky behavior (more than usual) :P

@SamSaffron
Copy link
Collaborator

It is holiday season, I will try to get a new stable out next week, but will only upgrade Discourse the week after

@SamSaffron
Copy link
Collaborator

thanks heaps @ignisf for the work on libv8 6.3 we now have a new release.

@Napolskih
Copy link

What about 6.4? They promise a significant optimization.

@ignisf
Copy link
Collaborator

ignisf commented Jan 25, 2018

I too am eager to push libv8 6.4. Just have to find some time to do so.

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

No branches or pull requests

5 participants