WIP: V8 upgrade

[georgyangelov]

Hey!

I started working on porting the C extension part of the code so that it works with V8 3.31 (the trunk branch in libv8). I'm creating this pull request to document my progress and for it to be of help if I get bored along the way.

The current idea is to start clean (no c or h files and no tests) and reimplement it via copy/paste and updating the parts that rely on the old V8 API. I also decided to create separate header files for each class as I am completely hopeless in navigating the current rr.h.

Current status: V8 objects can be created through Ruby. Still, there's a lot more work to be done.

Changes in V8 that affect the code:

• The main new thing is the v8::Isolate class. From what I understand:
  • It's a context for a V8 instance. It holds a reference to a V8 heap and state.
  • You can (and must) use it if you want to have two instances of V8 running at the same time.
  • Most of the V8 object constructors now require an isolate object as a parameter.
  • Just like Context, the Isolate can be entered and exited, and they can be stacked (you can enter multiple ones). You can currently get the entered isolate with v8::Isolate::GetCurrent(). However, from what I read, this method will be removed in the future so the isolate should be explicitly passed where necessary.
  • You must not use one isolate from more than one thread at the same time.
• v8::Persistent has some important changes as well:
  • The New method is removed in favor of the constructors.
  • It is now non-copyable (by default). This means that we need to hold a pointer.
  • It no longer inherits from Handle. To convert Persistent<T> to Handle<T> we need to use Local<T>::New
  • MakeWeak is now SetWeak and it requires a slightly different callback.
  • Dispose is now Reset
• String
  • NewSymbol is removed
  • New is removed in favor of NewFromUtf8, NewFromOneByte and NewFromTwoByte

Comments and help are most welcome! :)

[cowboyd]

Georgy,

This is fantastic! I would love to see this make it across the goal line, and will help you in whatever way I can!

If you are going to take on this effort, there are a couple of things to consider. First, I think we should remove all locking of V8 from Ruby entirely. Locking the V8::Isolate from the calling thread is something that should happen only from the C extension right before calling into V8. The reason is so that any locks that are being held by Ruby are released beforehand. For example, consider the following code:

V8::C::Locker::Locker() do
  cxt = V8::C::Context::New()
  # etc...
end

For all of the Ruby code between the do..end, both the V8 and Ruby locks are being held. This is the issue behind all kinds of deadlocks you can see scattered throughout the github issues. It's better to remove the ability to lock V8 from Ruby entirely and acquire the lock in C before each method call.

Also, if there are any classes you'd like to see documented, It's something I've been meaning to do, and if there were ever a time to do it, it's now :)

[georgyangelov]

Thank you for the comments!

I was trying to keep the C API as close to the original as possible, but I see now that there are some things that should change. I was thinking about how we may support multiple V8 isolates in different threads (since we will have isolate references anyway) and what would the C extension API look like for that. Unfortunately, I'm not sure how we can support this without changing the public API. Maybe the V8::Context can hold an isolate as well?

Also, what do you think of passing Context or Isolate objects from Ruby instead of relying on the 'current' ones? I'm trying to figure out if there is a case where the 'current' ones are different than the intended. It looks like V8 will soon require passing explicit context on object methods: http://v8.paulfryzel.com/docs/master/classv8_1_1_object.html

[cowboyd]

I think having public api evolve with the capabilities of V8 is ok, but I think we can keep the impact relatively low by having V8::Context create a new Isolate if one does not exist already:

# explicitly create Isolate
isolate = V8::Isolate.new
first = isolate.new_context()
second = isolate.new_context()

first.isolate == isolate #=> true
second.isolate == isolate #=> true

# implicily create Isolate
third = V8::Context.new
third.isolate == isolate #= false

Most folks can continue to use the implicit form with very little impact, but other the covers the situation where you might want more than one context. In both cases, the isolate is being passed around internally so that it's always available from C.

[georgyangelov]

I was thinking what is the best way to manage isolates and contexts. My current idea is to have every ref<T> object also hold a persistent handle to its context. The context itself has a GetIsolate method which can be used to create the lock, and the context object will be necessary for a lot of methods (http://v8.paulfryzel.com/docs/master/classv8_1_1_object.html). Unfortunately, to access the v8 Context from a persistent handle, we would need to pass it to v8::Local::New, which requires an isolate... I'm not sure if holding a third pointer (to the isolate) is ok.

My thinking with the references is that you wouldn't need to enter/exit isolates and contexts, which will allow seamless use of objects in different threads. Also, since the locks will be in the C extension, this would prevent another thread from accidentally switching the entered context in between C calls.

What do you think about this?

[cowboyd]

Sorry about the late response. You'll find that I'm very responsive Monday through Friday :)

I think it's ok and necessary that every ref<T> contain a pointer to the Isolate. The first step of any operation has to be to lock the thread to the ref.isolate;

It appears that objects can appear in multiple contexts, and the way forward for v8 is to pass the context in with every method, so it makes sense that we should follow that pattern at the V8::C::* level.

Remember, the pervasive philosophy of the C extension is that it should be (wherever possible) a 1:1 port to Ruby of the V8 C++ api. Then, we can layer on whatever API we need to from Ruby. I took this a bit too literally with the locking API though so it doesn't always apply.

What that means in practice is that ref<T> never has a pointer to a context, and instead for V8 operations, you pass in a ref<Context> just like any other parameter. For example, the following C++ method:

Maybe< bool > HasOwnProperty (Local< Context > context, Local< Name > key);

Would require 3 references. One ref<Object> (for self), one ref<Context>, and one ref<Name>

All of which would have a pointer to the same Isolate. Does that make sense?

[georgyangelov]

It's ok, I had other things to do this weekend anyway :)

Yes, it makes perfect sense. Thanks!

[cowboyd]

Were you able to make any progress on this during the week?

[georgyangelov]

No, unfortunately. I plan to now during the weekend.

[cowboyd]

@stormbreakerbg looks like you've been making some good progress. I was thinking: what if I try to add documentation as you go along, especially to the stranger classes?

It might help, and at the end, the code base would be well documented for the next set of re-work.

[georgyangelov]

@cowboyd Seems like a good idea. How do you want to do this? Do you want me to give you access to my fork?

[cowboyd]

Either that, or I could also just make it as a set of pull requests to your branch so that you can bring in the documentation where it makes sense.

[georgyangelov]

I'm ok either way. I think it will be simpler for you to just give you access. I added you as a collaborator.

[cowboyd]

I added some documentation for the equivalence classes and pointer classes, I'll try and get to some of the others today.

[cowboyd]

I added some documentation for the equivalence classes and pointer classes, I'll try and get to some of the others today.

[georgyangelov]

Thanks! Sorry about stopping mid-way. Hopefully I will be able to help with this again after the 15th this month (I will have a bachelor's degree final exam) but don't let that stop you from changes to the branch.

[cowboyd]

No worries! Hopefully I can fill in some of the stranger bits by then.

[cowboyd]

Due to the scope and complexity of this work: rewriting the internals of the ruby racer to account for almost 3 years of evolution in the V8 apis, I think the best course of action is to move development onto the main repo. The progress we've made has been really excellent, and represents a great distance travelled, but there is still a very long way to go

On the main repo, we can utilize the issue, wikis, and most importantly do the work in a more publicly visible arena where people who follow the project can track the progress. I'm going to take the work so far and push it to the upgrade-to-v8-4.5 . Then, I will open the currently pending pull requests against that branch. the [upgrade-to-v8-4.5] branch will become the target for The Ruby Racer 1.0

https://github.com/cowboyd/therubyracer/pull/348