Skip to content
This repository was archived by the owner on Dec 4, 2023. It is now read-only.

Update upstream V8 to 6.0 #243

Closed
ignisf opened this issue Jul 26, 2017 · 29 comments
Closed

Update upstream V8 to 6.0 #243

ignisf opened this issue Jul 26, 2017 · 29 comments
Assignees
Milestone

Comments

@ignisf
Copy link
Collaborator

ignisf commented Jul 26, 2017

No description provided.

@ignisf ignisf self-assigned this Jul 26, 2017
@ignisf
Copy link
Collaborator Author

ignisf commented Jul 26, 2017

cc @SamSaffron

@SamSaffron
Copy link
Contributor

❤️ love that you are keeping this up to date!

@ignisf
Copy link
Collaborator Author

ignisf commented Jul 26, 2017

I just know you like to run benchmarks, so I'm giving you an excuse to ;)

Will release beta as soon as I see the CI green.

@SamSaffron
Copy link
Contributor

awesome, we are going to want to test this quick smart given:

https://twitter.com/bmeurer/status/890247544764289025

v8 6.0 was released yesterday 🎊

@SamSaffron
Copy link
Contributor

cc @seanmakesgames

@seanmakesgames
Copy link

Interesting. I wish it was easier to benchmark the 'real world' code in my game without pushing it live. I wonder if waiting until 6.1 is released is the right bet. 5.7 has been treating me really well wrt stability (much more important than performance & features in my use case)

@SamSaffron
Copy link
Contributor

I think 6.0 should in theory help, will bench it and see. I do not expect any stability issues though, just minor perf issues.

@seanmakesgames
Copy link

haha, in this project, if 'tie-scenario' changes in the underlying sort implementation (apparently not specified in js standard) or something like the random number impl; it can and will break so many things.
Luckily the sandboxy stuff is all pretty well tested, so I'll know if my lockdown methods are immediately compromised. ;)
In any case, the upgrade from 5.3 to 5.7 completely halted segfaults (was still getting those on production) and the only reason any of my workers are dying now is unbounded mem growth.

anyhow, over-details. I'll probably take a stab at 6.0 and rollback if necessary. :)

@ignisf
Copy link
Collaborator Author

ignisf commented Jul 27, 2017

Beta is up.

@ignisf
Copy link
Collaborator Author

ignisf commented Jul 27, 2017

need to fix #244 before release though

@seanmakesgames
Copy link

not sure if this is a v8 issue or miniracer issue, but I'm getting this on a rake clean compile test in mini_racer:

/Users/u/mini_racer/lib/mini_racer.rb:2:in `require': dlopen(/Users/u/mini_racer/lib/mini_racer_extension.bundle, 9): Symbol not found: __ZTIN2v811ArrayBuffer9AllocatorE (LoadError)
  Referenced from: /Users/u/mini_racer/lib/mini_racer_extension.bundle
  Expected in: flat namespace
 in /Users/u/mini_racer/lib/mini_racer_extension.bundle - /Users/u/mini_racer/lib/mini_racer_extension.bundle
	from /Users/u/mini_racer/lib/mini_racer.rb:2:in `<top (required)>'
	from /Users/u/mini_racer/test/test_helper.rb:2:in `require'
	from /Users/u/mini_racer/test/test_helper.rb:2:in `<top (required)>'
	from /Users/u/mini_racer/test/mini_racer_test.rb:3:in `require'
	from /Users/u/mini_racer/test/mini_racer_test.rb:3:in `<top (required)>'
	from /Users/u/.rvm/gems/ruby-2.2.4/gems/rake-10.5.0/lib/rake/rake_test_loader.rb:15:in `require'
	from /Users/u/.rvm/gems/ruby-2.2.4/gems/rake-10.5.0/lib/rake/rake_test_loader.rb:15:in `block in <main>'
	from /Users/u/.rvm/gems/ruby-2.2.4/gems/rake-10.5.0/lib/rake/rake_test_loader.rb:4:in `select'
	from /Users/u/.rvm/gems/ruby-2.2.4/gems/rake-10.5.0/lib/rake/rake_test_loader.rb:4:in `<main>'
rake aborted!
Command failed with status (1): [ruby -I"lib:test:lib" -I"/Users/u/.rvm/gems/ruby-2.2.4/gems/rake-10.5.0/lib" "/Users/u/.rvm/gems/ruby-2.2.4/gems/rake-10.5.0/lib/rake/rake_test_loader.rb" "test/mini_racer_test.rb" ]

@SamSaffron
Copy link
Contributor

confirmed @ignisf appears I am having trouble linking this with miniracer

/home/sam/Source/mini_racer/lib/mini_racer.rb:2:in `require': /home/sam/Source/mini_racer/lib/mini_racer_extension.so: undefined symbol: _ZTIN2v811ArrayBuffer9AllocatorE - /home/sam/Source/mini_racer/lib/mini_racer_extension.so (LoadError)

Maybe arraybufferallocator got removed or something?

@ignisf
Copy link
Collaborator Author

ignisf commented Jul 28, 2017

confirmed on linux, too. Looking into it.

@ignisf
Copy link
Collaborator Author

ignisf commented Jul 28, 2017

I screwed up with the patches...

@ignisf ignisf added this to the 6.0.286.44 milestone Jul 28, 2017
@seanmakesgames
Copy link

anything we can do to help with this stuff?

@ignisf
Copy link
Collaborator Author

ignisf commented Aug 3, 2017

Hya,

I've been exceptionally busy lately. Unfortunately it's not the patches. I've been hitting this issue even with them removed D:. Still investigating...

Things left to do:

  1. Check the newer 6.0 version that Google uses.
  2. Remove the parameters that builder.rb passes to the make command.

@seanmakesgames
Copy link

yikes! That's weird..
No problem re: busy @ignisf - I'm gonna bite the bullet and use 5.9
Thanks for your efforts on this stuff!

@ignisf
Copy link
Collaborator Author

ignisf commented Aug 3, 2017

No luck with the newer version... And without any params the build fails even before it has started... D:

@ignisf
Copy link
Collaborator Author

ignisf commented Aug 22, 2017

@seanmakesgames @SamSaffron, I'm beginning to think it's a breaking change in upstream V8 causing this. Not sure what yet.

Working:

[ignisf@clarity x64.release]$ readelf -Ws /home/ignisf/Projects/mini_racer/tmp/x86_64-linux/stage/lib/mini_racer_extension.so | grep --color 'ArrayBuffer.*AllocatorE'
 10451: 00000000012ce000    40 OBJECT  WEAK   DEFAULT   21 _ZTVN2v88internal31JSArrayBufferDataEntryAllocatorE
 10590: 00000000012ab3a0    56 OBJECT  WEAK   DEFAULT   21 _ZTVN2v811ArrayBuffer9AllocatorE
 18084: 0000000000cdb330    29 OBJECT  WEAK   DEFAULT   14 _ZTSN2v811ArrayBuffer9AllocatorE
 19187: 00000000012ab2c0    16 OBJECT  WEAK   DEFAULT   21 _ZTIN2v811ArrayBuffer9AllocatorE
 28174: 00000000005462d0    29 FUNC    GLOBAL DEFAULT   12 _ZN2v811ArrayBuffer9Allocator19NewDefaultAllocatorEv
    73: 000000000129a340    56 OBJECT  LOCAL  DEFAULT   21 _ZTVN2v812_GLOBAL__N_120ArrayBufferAllocatorE
 18757: 0000000000cdb330    29 OBJECT  WEAK   DEFAULT   14 _ZTSN2v811ArrayBuffer9AllocatorE
 22310: 00000000012ab3a0    56 OBJECT  WEAK   DEFAULT   21 _ZTVN2v811ArrayBuffer9AllocatorE
 29936: 00000000012ce000    40 OBJECT  WEAK   DEFAULT   21 _ZTVN2v88internal31JSArrayBufferDataEntryAllocatorE
 31778: 00000000005462d0    29 FUNC    GLOBAL DEFAULT   12 _ZN2v811ArrayBuffer9Allocator19NewDefaultAllocatorEv
 33625: 00000000012ab2c0    16 OBJECT  WEAK   DEFAULT   21 _ZTIN2v811ArrayBuffer9AllocatorE

Not working:

[ignisf@clarity x64.release]$ readelf -Ws /home/ignisf/Projects/mini_racer/tmp/x86_64-linux/stage/lib/mini_racer_extension.so | grep --color 'ArrayBuffer.*AllocatorE'
    67: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND _ZTIN2v811ArrayBuffer9AllocatorE
  7605: 0000000000ffbfb8    40 OBJECT  WEAK   DEFAULT   21 _ZTVN2v88internal31JSArrayBufferDataEntryAllocatorE
  7694: 0000000000fdc918    80 OBJECT  GLOBAL DEFAULT   21 _ZTVN2v811ArrayBuffer9AllocatorE
 20402: 0000000000399c70    23 FUNC    GLOBAL DEFAULT   12 _ZN2v811ArrayBuffer9Allocator19NewDefaultAllocatorEv
   125: 0000000000fdc9a8    80 OBJECT  LOCAL  DEFAULT   21 _ZTVN2v812_GLOBAL__N_120ArrayBufferAllocatorE
  4656: 0000000000ffbfb8    40 OBJECT  WEAK   DEFAULT   21 _ZTVN2v88internal31JSArrayBufferDataEntryAllocatorE
  9283: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND _ZTIN2v811ArrayBuffer9AllocatorE
 16208: 0000000000fdc918    80 OBJECT  GLOBAL DEFAULT   21 _ZTVN2v811ArrayBuffer9AllocatorE
 20198: 0000000000399c70    23 FUNC    GLOBAL DEFAULT   12 _ZN2v811ArrayBuffer9Allocator19NewDefaultAllocatorEv
[ignisf@clarity x64.release]$

Tried to make the linker tell me what's wrong by making it produce a static library:

[ignisf@clarity mini_racer]$ g++ -static -o mini_racer_extension.a mini_racer_extension.o -L. -L/home/ignisf/.rbenv/versions/2.3.3/lib -Wl,-R/home/ignisf/.rbenv/versions/2.3.3/lib /home/ignisf/.rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/gems/libv8-6.1.534.22.0beta1-x86_64-linux/vendor/v8/out/x64.release/libv8_base.a /home/ignisf/.rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/gems/libv8-6.1.534.22.0beta1-x86_64-linux/vendor/v8/out/x64.release/libv8_libbase.a /home/ignisf/.rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/gems/libv8-6.1.534.22.0beta1-x86_64-linux/vendor/v8/out/x64.release/libv8_snapshot.a /home/ignisf/.rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/gems/libv8-6.1.534.22.0beta1-x86_64-linux/vendor/v8/out/x64.release/libv8_libplatform.a /home/ignisf/.rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/gems/libv8-6.1.534.22.0beta1-x86_64-linux/vendor/v8/out/x64.release/libv8_libsampler.a -L. -L/home/ignisf/.rbenv/versions/2.3.3/lib  -fstack-protector -rdynamic -Wl,-export-dynamic    -lpthread  -lpthread -ldl -lcrypt -lm   -lc 2>&1 | grep ArrayB
mini_racer_extension.o:(.data.rel.ro._ZTI20ArrayBufferAllocator[_ZTI20ArrayBufferAllocator]+0x10): undefined reference to `typeinfo for v8::ArrayBuffer::Allocator'

@ignisf
Copy link
Collaborator Author

ignisf commented Aug 22, 2017

6.1:

/**
 * An instance of the built-in ArrayBuffer constructor (ES6 draft 15.13.5).
 */
class V8_EXPORT ArrayBuffer : public Object {
 public:
  /**
   * A thread-safe allocator that V8 uses to allocate |ArrayBuffer|'s memory.
   * The allocator is a global V8 setting. It has to be set via
   * Isolate::CreateParams.
   *
   * Memory allocated through this allocator by V8 is accounted for as external
   * memory by V8. Note that V8 keeps track of the memory for all internalized
   * |ArrayBuffer|s. Responsibility for tracking external memory (using
   * Isolate::AdjustAmountOfExternalAllocatedMemory) is handed over to the
   * embedder upon externalization and taken over upon internalization (creating
   * an internalized buffer from an existing buffer).
   *
   * Note that it is unsafe to call back into V8 from any of the allocator
   * functions.
   */
  class V8_EXPORT Allocator { // NOLINT
   public:
    virtual ~Allocator() {}

    /**
     * Allocate |length| bytes. Return NULL if allocation is not successful.
     * Memory should be initialized to zeroes.
     */
    virtual void* Allocate(size_t length) = 0;

    /**
     * Allocate |length| bytes. Return NULL if allocation is not successful.
     * Memory does not have to be initialized.
     */
    virtual void* AllocateUninitialized(size_t length) = 0;

    /**
     * Reserved |length| bytes, but do not commit the memory. Must call
     * |SetProtection| to make memory accessible.
     */
    // TODO(eholk): make this pure virtual once blink implements this.
    virtual void* Reserve(size_t length);

    /**
     * Free the memory block of size |length|, pointed to by |data|.
     * That memory is guaranteed to be previously allocated by |Allocate|.
     */
    virtual void Free(void* data, size_t length) = 0;

    enum class AllocationMode { kNormal, kReservation };

    /**
     * Free the memory block of size |length|, pointed to by |data|.
     * That memory is guaranteed to be previously allocated by |Allocate| or
     * |Reserve|, depending on |mode|.
     */
    // TODO(eholk): make this pure virtual once blink implements this.
    virtual void Free(void* data, size_t length, AllocationMode mode);

    enum class Protection { kNoAccess, kReadWrite };

    /**
     * Change the protection on a region of memory.
     *
     * On platforms that make a distinction between reserving and committing
     * memory, changing the protection to kReadWrite must also ensure the memory
     * is committed.
     */
    // TODO(eholk): make this pure virtual once blink implements this.
    virtual void SetProtection(void* data, size_t length,
                               Protection protection);

    /**
     * malloc/free based convenience allocator.
     *
     * Caller takes ownership, i.e. the returned object needs to be freed using
     * |delete allocator| once it is no longer in use.
     */
    static Allocator* NewDefaultAllocator();
  };

5.9:

/**
 * An instance of the built-in ArrayBuffer constructor (ES6 draft 15.13.5).
 */
class V8_EXPORT ArrayBuffer : public Object {
 public:
  /**
   * A thread-safe allocator that V8 uses to allocate |ArrayBuffer|'s memory.
   * The allocator is a global V8 setting. It has to be set via
   * Isolate::CreateParams.
   *
   * Memory allocated through this allocator by V8 is accounted for as external
   * memory by V8. Note that V8 keeps track of the memory for all internalized
   * |ArrayBuffer|s. Responsibility for tracking external memory (using
   * Isolate::AdjustAmountOfExternalAllocatedMemory) is handed over to the
   * embedder upon externalization and taken over upon internalization (creating
   * an internalized buffer from an existing buffer).
   *
   * Note that it is unsafe to call back into V8 from any of the allocator
   * functions.
   */
  class V8_EXPORT Allocator { // NOLINT
   public:
    virtual ~Allocator() {}

    /**
     * Allocate |length| bytes. Return NULL if allocation is not successful.
     * Memory should be initialized to zeroes.
     */
    virtual void* Allocate(size_t length) = 0;

    /**
     * Allocate |length| bytes. Return NULL if allocation is not successful.
     * Memory does not have to be initialized.
     */
    virtual void* AllocateUninitialized(size_t length) = 0;

    /**
     * Free the memory block of size |length|, pointed to by |data|.
     * That memory is guaranteed to be previously allocated by |Allocate|.
     */
    virtual void Free(void* data, size_t length) = 0;

    /**
     * malloc/free based convenience allocator.
     *
     * Caller takes ownership, i.e. the returned object needs to be freed using
     * |delete allocator| once it is no longer in use.
     */
    static Allocator* NewDefaultAllocator();
  };

@ignisf
Copy link
Collaborator Author

ignisf commented Aug 22, 2017

Hmmm...

This seems to fix it...

diff --git a/ext/mini_racer_extension/mini_racer_extension.cc b/ext/mini_racer_extension/mini_racer_extension.cc
index 63c75d4..6d6f43c 100644
--- a/ext/mini_racer_extension/mini_racer_extension.cc
+++ b/ext/mini_racer_extension/mini_racer_extension.cc
@@ -11,20 +11,6 @@
 
 using namespace v8;
 
-class ArrayBufferAllocator : public v8::ArrayBuffer::Allocator {
- public:
-  virtual void* Allocate(size_t length) {
-    void* data = AllocateUninitialized(length);
-    return data == NULL ? data : memset(data, 0, length);
-  }
-  virtual void* AllocateUninitialized(size_t length) {
-    return malloc(length);
-  }
-  virtual void Free(void* data, size_t) {
-     free(data);
-  }
-};
-
 typedef struct {
     const char* data;
     int raw_size;
@@ -32,7 +18,7 @@ typedef struct {
 
 typedef struct {
     Isolate* isolate;
-    ArrayBufferAllocator* allocator;
+    v8::ArrayBuffer::Allocator* allocator;
     StartupData* startup_data;
     bool interrupted;
     bool disposed;
@@ -453,7 +439,7 @@ static VALUE rb_isolate_init_with_snapshot(VALUE self, VALUE snapshot) {
 
     init_v8();
 
-    isolate_info->allocator = new ArrayBufferAllocator();
+    isolate_info->allocator = v8::ArrayBuffer::Allocator::NewDefaultAllocator();
     isolate_info->interrupted = false;
     isolate_info->refs_count = 1;

@SamSaffron
Copy link
Contributor

SamSaffron commented Aug 22, 2017 via email

@ignisf
Copy link
Collaborator Author

ignisf commented Aug 22, 2017

Opened rubyjs/mini_racer#64

@ignisf
Copy link
Collaborator Author

ignisf commented Aug 22, 2017

mini_racer now works with the 6.0 beta

@ignisf
Copy link
Collaborator Author

ignisf commented Aug 24, 2017

v6.0.286.54.0beta1 is up

@ignisf
Copy link
Collaborator Author

ignisf commented Aug 24, 2017

v6.1.534.22.0beta1 will soon be up, too

@ignisf ignisf closed this as completed Aug 24, 2017
@xaxxon
Copy link

xaxxon commented Sep 6, 2017

did you guys ever figure out what changed that caused this problem in the first place? I'm running into the v8::ArrayBufferAllocator typeinfo problem myself.. though I'll probably swap to the convenience allocator, too

@ignisf
Copy link
Collaborator Author

ignisf commented Sep 6, 2017

@xaxxon, hey

Yes -- as far as I understood, the underlying V8 interface for the Allocator changed and we had unimplemented virtual methods as a result of this. (I love how I needed to debug a cryptic linking error to get to this).

Just changing to the default implementation allows us to not worry about them changing the interface again, hopefully.

@xaxxon
Copy link

xaxxon commented Sep 6, 2017 via email

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

No branches or pull requests

4 participants