Making v8::Persistent safe to use

[dca...@chromium.org]

Hi,

due to fundamental problems in the design of the Persistent handle API, we feel the need to considerably change the V8 API to ensure that the Persistent class is safe to use.  Currently, it is extremely difficult to use safely and can easily lead to heap corruption.  To get an understanding of the problem, see https://code.google.com/p/v8/issues/detail?id=1889.

At the moment we’re considering the following major changes:

• Making Persistent an independent class (no longer deriving from Handle).

  
  

• Deprecating Local and eventually removing it in favor of just having Handle.

  
  

• Changing all callbacks that return Handle<Value> to return void and instead pass the return value in as a parameter.

  
  

The last change is not inherently required by the changes to Persistent but rather to maintain performant callbacks which return Persistent.  The old style of callbacks we want to be deprecate and eventually remove.  In the meantime, old style callbacks can support the return value parameter when an empty handle is returned, providing a transition path.

I have created two patches that attempt this transformation (most of it anyway).  They are https://codereview.chromium.org/12729023/ and https://codereview.chromium.org/12494012/.

Not everything is yet set in stone, and I expect a certain amount of change to the overall implementation path as we work through implementation issues and receive feedback.

Dan

[Trevor Norris]

Glad to see Persistent getting so much love. I'd just like to drop my 2 cents worth of pain recently.

For better performance I've created an allocator that grabs a large chunk of memory, divides it and hands those off via SetIndexedPropertiesToExternalArrayData. The minimum number of steps I've found to accomplish this are broken into two parts. First for a new large chunk:

• Create a new Persistent<Object> with an Object::New(), in the global scope. This basically acts as a placeholder for v8's gc.
• MakeWeak and pass the char* as the parameter
• AdjustAmountOfExternalAllocatedMemory

Then for allocating a chunk:

• SetIndexedPropertiesToExternalArrayData
  
• Then, to make sure v8 knows when to gc the Persistent, attach the global Persistent to the js object via SetHiddenValue, SetInternalField, etc.

Now when no more js objects are pointing to the Persistent, it will call the MakeWeakCallback. Which will then perform cleanup, etc.

There are two non-trivial issues with this approach. First; if js objects hang around pointing to small parts of the data then large portions hang around unused. Second; this process is extremely expensive, and much more so if each object were to be made it's own Persistent for independent tracking.

So what I'd like for consideration are:

• Creation of a much less expensive API for attaching a Persistent to a js object for the explicit purpose of knowing when to gc.
• A way to track what js objects are still pointing to memory. Possibly like a method on Persistent that allows to step through all attached js objects.

If these are currently possible with the existing API I apologize, but so far I haven't been able to find a way to allocate many small-ish chunks of external memory w/o slowing down performance considerably.

trev

[Stephan Beal]

On Mon, Mar 25, 2013 at 2:47 PM, <dca...@chromium.org> wrote:

At the moment we’re considering the following major changes:

• Making Persistent an independent class (no longer deriving from Handle).

  
  

• Deprecating Local and eventually removing it in favor of just having Handle.

  
  

• Changing all callbacks that return Handle<Value> to return void and instead pass the return value in as a parameter.

   

We all certainly appreciate the head's up (it's a first, AFAIR - normally we find out about any significant changes the hard way).

Does point #3 really mean that the signature of v8::InvocationCallback will change?

--
----- stephan beal
http://wanderinghorse.net/home/stephan/

http://gplus.to/sgbeal

[Trevor Norris]

On Wednesday, March 27, 2013 11:45:35 AM UTC-7, Trevor Norris wrote:

Glad to see Persistent getting so much love. I'd just like to drop my 2 cents worth of pain recently.

Ignore my pain. Happens that the examples I was using were for a special case, and overkill for my simple allocator. Once I figured out how to simplify the code it ran much more efficiently.

trev

[Dan Carney]

A quick update:

I've committed the changes to deprecate all old style callbacks in favour of the new style which return void.  As of version 3.20, using old style callbacks will issue deprecation warnings but they will continue to be in the api and work for some number of versions to come to allow for a transition path for embedders.

Please see the following patches for examples of how to convert between old and new styles:

https://chromiumcodereview.appspot.com/17064004/

https://chromiumcodereview.appspot.com/17071004/

https://chromiumcodereview.appspot.com/17336003/

[Ben Noordhuis]

Would it be possible to rename GetReturnValue() to ReturnValue()?
Embedders will be typing that a lot so briefer is better IMO.

[Trevor Norris]

On Thursday, June 20, 2013 7:42:21 AM UTC-7, Dan Carney wrote:

A quick update:

I've committed the changes to deprecate all old style callbacks in favour of the new style which return void.  As of version 3.20, using old style callbacks will issue deprecation warnings but they will continue to be in the api and work for some number of versions to come to allow for a transition path for embedders.

Is there a timeline when the many items marked for deprecation will be official? Most specifically Local<> in favor of Handle<>. 

[Dan Carney]

The transition from Local to Handle won't happen for a while.  It's more of a cleanup step after everything else is done, and there's no urgency since there shouldn't be any performance impact.  At that point, we'll add some cutover functions and #defines to make transition easier.

[Stephan Beal]

On Fri , Jun 21, 2013 at 9:19 AM, Dan Carney <dca...@chromium.org> wrote:

The transition from Local to Handle won't happen for a while.  It's more of a cleanup step after everything else is done, and there's no urgency since there shouldn't be any performance impact.

Correction: there is no urgency for Chrome. There _are_ hundreds of projects out there based on v8 other than Chrome, and none of them can plan for fixing their code which are broken by this series of changes.

The callback signature changes alone break almost every single line of v8-using code i've written (tens of thousands of them), and i am still undecided as to whether to spend several weeks of my time patching for or whether to simply drop by v8-related projects (i'm leaning strongly towards the latter). i'm getting too old to spend weeks patching every time a 3rd-party library pulls the rug out from under me (and this isn't the first time v8 has done it, though this is certainly the most invasive change so far).

>:(

[Dan Carney]

The callback signature changes alone break almost every single line of v8-using code i've written (tens of thousands of them), and i am still undecided as to whether to spend several weeks of my time patching for or whether to simply drop by v8-related projects (i'm leaning strongly towards the latter). i'm getting too old to spend weeks patching every time a 3rd-party library pulls the rug out from under me (and this isn't the first time v8 has done it, though this is certainly the most invasive change so far).

 

I feel your pain.  Converting chrome was a large amount of tedious work.

Ultimately, though, the v8 team does not want to support two APIs for any sustained amount of time.  I've put up a header only library on github that embedders can use if they do not want to rewrite all their callbacks: https://github.com/drcarney66/v8-backport.  You will, however, have to install wrap<your_old_function_name> at callback registration time instead of your_old_function_name.  This far from a perfect solution, but should help in a lot of cases, and it's certainly less work that manually converting a zillion functions.  Note that performance sensitive functions should be converted.

Additionally, as things like Arguments, InvocationCallback, and AccessorInfo are removed from v8.h, I'll just create a compatible version in the header which should keep things compiling for anyone choosing to use that library.

One further note: instead of removing Local, Handle is going to be removed instead, which should cause embedders a lot less pain.

[Stephan Beal]

On Tue, Jun 25, 2013 at 3:01 PM, Dan Carney <dca...@chromium.org> wrote:

I feel your pain.  Converting chrome was a large amount of tedious work.

With a large team. My v8-based projects have an active team of one with some 5 years of accumulated code based (centered!) on the InvocationCallback interface. :/

Ultimately, though, the v8 team does not want to support two APIs for any sustained amount of time.  I've put up a header only library on github that embedders can use if they do not want to rewrite all their callbacks: https://github.com/drcarney66/v8-backport.  You will, however, have to install wrap<your_old_function_name> at callback registration time 

instead of your_old_function_name.  This far from a perfect solution, but should help in a lot of cases, and it's certainly less work that manually converting a zillion functions.  Note that performance sensitive functions should be converted.

i will certainly be taking a look at that. Maybe i can use it as a pattern for porting even if i can't use it directly. My code is primarily library-level templates which convert arbitrary client-side functions to InvocationCallbacks (http://code.google.com/p/v8-juice/wiki/V8Convert_XTo), so these changes really hit me where it hurts.

 

Additionally, as things like Arguments, InvocationCallback, and AccessorInfo are removed from v8.h, I'll just create a compatible version in the header which should keep things compiling for anyone choosing to use that library.

i honestly don't think i can continue my own v8 projects without a path like this one.

One further note: instead of removing Local, Handle is going to be removed instead, which should cause embedders a lot less pain.

i've never understood the difference, anyway - i just used whatever v8 called for at the time. Globally replacing these is easy compared to changing a function's signature.

Thanks for the feedback, and Happy Hacking...

-- 

[Trevor Norris]

> One further note: instead of removing Local, Handle is going to be removed instead, which should cause embedders a lot less pain.

Interesting. Thanks for the heads up. Does this mean Undefined/True/False etc. will return Local<Value>?

[Dan Carney]

Interesting. Thanks for the heads up. Does this mean Undefined/True/False etc. will return Local<Value>?

Yes, it will.  I'm not entirely sure why it ever returned Handle when pretty much the rest of the api uses Local exclusively.  Do you envision that causing a problem for you somehow?

[Trevor Norris]

On Jun 25, 2013 8:38 AM, "Dan Carney" <dca...@chromium.org> wrote:
>>
>> Interesting. Thanks for the heads up. Does this mean Undefined/True/False etc. will return Local<Value>?
>
> Yes, it will.  I'm not entirely sure why it ever returned Handle when pretty much the rest of the api uses Local exclusively.  Do you envision that causing a problem for you somehow?

Not at all. AFAIK you don't have to use scope.Close() when returning those values so the nomenclature just might seem a little strange. That's all.

[Dan Carney]

Not at all. AFAIK you don't have to use scope.Close() when returning those values so the nomenclature just might seem a little strange. That's all.

I see. The HandleScope::Close behaviour will remain, but it was always just an implementation detail, technically not something one should rely on. I'm not sure if the signature of those functions was supposed to indicate that or not.

[Dan Carney]

i will certainly be taking a look at that. Maybe i can use it as a pattern for porting even if i can't use it directly. My code is primarily library-level templates which convert arbitrary client-side functions to InvocationCallbacks (http://code.google.com/p/v8-juice/wiki/V8Convert_XTo), so these changes really hit me where it hurts.

Wow. Yeah, total conversion doesn't look like a great option for you.  I really hope a small amount of additional template magic can work around your problems.

[Dan Carney]

Would it be possible to rename GetReturnValue() to ReturnValue()?
Embedders will be typing that a lot so briefer is better IMO.

It's a little late for a rename, I'm afraid.  If you have a lot of stuff to convert, I'd recommend wrapping the returning of values with some helper templates.  I did that when converting blink, and it was very useful see: http://cs.chromium.org/v8SetReturnValue.

[Ben Noordhuis]

On Fri, Jun 21, 2013 at 9:19 AM, Dan Carney <dca...@chromium.org> wrote:
> The transition from Local to Handle won't happen for a while. It's more of
> a cleanup step after everything else is done, and there's no urgency since
> there shouldn't be any performance impact. At that point, we'll add some
> cutover functions and #defines to make transition easier.

Dan, Handle and Local contain these snippets:

// TODO(dcarney): remove before cutover
V8_INLINE(static Handle<T> New(Isolate* isolate, const
Persistent<T>& that)) {
return New(isolate, that.val_);
}

And:

// TODO(dcarney): remove before cutover
V8_INLINE(static Local<T> New(Isolate* isolate, const Persistent<T>& that));

Is that comment in error? Put another way: if or when those functions
are removed, how are you supposed to get at the handle that the
Persistent points to?

Another thing is Persistent<T>::ClearAndLeak(). Here is its definition:

V8_INLINE(T* ClearAndLeak());

What purpose does it serve? You can't construct a Handle<T> from a
raw T*, all constructors and New() class methods that take a T* are
private.

[Dan Carney]

Is that comment in error?  Put another way: if or when those functions
are removed, how are you supposed to get at the handle that the
Persistent points to?

Yes, it's an error in the comment.  Those functions will not be removed.

 

Another thing is Persistent<T>::ClearAndLeak().  Here is its definition:

  V8_INLINE(T* ClearAndLeak());

What purpose does it serve?  You can't construct a Handle<T> from a
raw T*, all constructors and New() class methods that take a T* are
private.

We use it internally to embed persistents in collections.  We were planning on having some API function for using it, but have recently decided against that plan.  It will be removed.

[Trevor Norris]

Ben just landed the 3.20.x upgrade to Node, and I'm noticing a non-trivial performance regression using Persistent<>. Before it was fast to cache objects that would be passed to Function::Call as Persistent<Object>, but now it seems to require a Local<Object>::New(Isolate*, Persistent<Object>) before being sent. For quick calls out of cc to js it's adding ~10% to call time. Is there a way around this regression that I'm not seeing?

--
--
v8-users mailing list
v8-u...@googlegroups.com
http://groups.google.com/group/v8-users
---
You received this message because you are subscribed to a topic in the Google Groups "v8-users" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/v8-users/6kSAbnUb-rQ/unsubscribe.
To unsubscribe from this group and all its topics, send an email to v8-users+u...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

[Dan Carney]

On Mon, Jul 8, 2013 at 5:43 AM, Trevor Norris <trev....@gmail.com> wrote:

Ben just landed the 3.20.x upgrade to Node, and I'm noticing a non-trivial performance regression using Persistent<>. Before it was fast to cache objects that would be passed to Function::Call as Persistent<Object>, but now it seems to require a Local<Object>::New(Isolate*, Persistent<Object>) before being sent. For quick calls out of cc to js it's adding ~10% to call time. Is there a way around this regression that I'm not seeing?

I'm not quite sure what you mean.  Do you mean you want to return a value stored in a Persistent? If so, that's done via GetReturnValue().Set(Persistent&).  That should take care of the regression if that's the case.  If not, could you provide me with an example?

[Trevor Norris]

On Mon, Jul 8, 2013 at 12:53 AM, Dan Carney <dca...@chromium.org> wrote:

I'm not quite sure what you mean.  Do you mean you want to return a value stored in a Persistent? If so, that's done via GetReturnValue().Set(Persistent&).  That should take care of the regression if that's the case.  If not, could you provide me with an example?

Say for example the Persistent<Object>* passed to the MakeWeak callback. Before 3.20 it was possible to  Object::GetIndexedPropertiesExternalArrayDataLength() directly on the Persistent, but now it's necessary to Local<Object>::New() first. Or a worse case is where both a Function and Object are cached as Persistent<T> it's then necessary to do something like the following:

Local<Function>::New(isolate, p_fn)->Call(Local<Object>::New(isolate, p_obj), argc, argv[]);

Constantly needing to create a Local<T>::New() every time I need to use a Persistent<T> in cases like the above isn't playing nicely. I've noticed since the change that GC time from --prof has grown around 5% in high I/O scenarios.

[Dan Carney]

Say for example the Persistent<Object>* passed to the MakeWeak callback. Before 3.20 it was possible to  Object::GetIndexedPropertiesExternalArrayDataLength() directly on the Persistent, but now it's necessary to Local<Object>::New() first. Or a worse case is where both a Function and Object are cached as Persistent<T> it's then necessary to do something like the following:

Local<Function>::New(isolate, p_fn)->Call(Local<Object>::New(isolate, p_obj), argc, argv[]);

I'm afraid that, if you use 3.20, for the cases you've mentioned, you're stuck with that performance regression if you use the API correctly.  While cutting Chrome over to this API, we had to use a lot of reinterpret_casts<Local*> to work around some of these problems.

The weak callback problem we've already discovered and are in the process of fixing, but I'm not sure when the new weak callback API will arrive.

For certain API functions, we can create a version that's callable from a Persistent directly, but Function::Call unfortunately isn't one of them.  We have a few functions slated to have a Persistent version, like GetAlignedPointerFromInternalField, and if there are certain functions causing you pain, we can look at adding them if you send me a list.  If there's any path by which a function might allocate heap memory, however, we cannot add it.  This actually takes most functions out of contention.

[Trevor Norris]

On Mon, Jul 8, 2013 at 4:23 AM, Dan Carney <dca...@chromium.org> wrote:

I'm afraid that, if you use 3.20, for the cases you've mentioned, you're stuck with that performance regression if you use the API correctly.  While cutting Chrome over to this API, we had to use a lot of reinterpret_casts<Local*> to work around some of these problems. 

The weak callback problem we've already discovered and are in the process of fixing, but I'm not sure when the new weak callback API will arrive.

For certain API functions, we can create a version that's callable from a Persistent directly, but Function::Call unfortunately isn't one of them.

Right now the largest pain is needing to Local<T>::New() the Persistent<T>* passed to the MakeWeak callback so we can access external memory attached via Object::SetIndexedPropertiesToExternalArrayData().

As for using reinterpret_cast<Local*>, will that be a long-er term solution for problems like the following:

Persistent<Function> p_call_this_often;

Persistent<Object> p_empty_obj;

template<class T>

inline Local<T> ToLocal(Persistent<T>* p_) {

  return *reinterpret_cast<Local<T>*>(p_);

}

void CallMe(const FunctionCallbackInfo<Value>& args) {

  HandleScope scope(args.GetCurrentIsolate());

  // storing and passing a cached empty object is the fastest way I've found

  // to call a js fn from cc

  ToLocal<Function>(&p_call_this_often)->Call(ToLocal<Object>(&p_empty_obj), argc, argv);

}

Using ::New() in a couple specific cases like this leads to ~15% regression.

 

We have a few functions slated to have a Persistent version, like GetAlignedPointerFromInternalField,

To be honest I stay far away from Object::Set, Object::SetHiddenValue, etc. wherever I can. Performance tests have shown me that it's usually faster to set properties by calling out to js first (for non-hidden/readonly, etc). Here are the specific performance tests I'm referring to: https://github.com/trevnorris/talks/tree/master/js-vs-cc-set-properties

They've shown me it's ~15x's faster to call out to js from cc to create an object and set three properties than to do the same from cc. So being able to call out to js as quickly as possible has been very important to creating performant code.

 

and if there are certain functions causing you pain, we can look at adding them if you send me a list.  If there's any path by which a function might allocate heap memory, however, we cannot add it.

Right now they'd be:

- Object::HasIndexedPropertiesInExternalArrayData()

- Object::GetIndexedPropertiesExternalArrayData()

- Object::GetIndexedPropertiesExternalArrayDataLength()

[Dan Carney]

Right now the largest pain is needing to Local<T>::New() the Persistent<T>* passed to the MakeWeak callback so we can access external memory attached via Object::SetIndexedPropertiesToExternalArrayData().

Okay, that's good, as we have a fix already lined up for that problem.

 

As for using reinterpret_cast<Local*>, will that be a long-er term solution for problems like the following:

void CallMe(const FunctionCallbackInfo<Value>& args) {

  HandleScope scope(args.GetCurrentIsolate());

  // storing and passing a cached empty object is the fastest way I've found

  // to call a js fn from cc

  ToLocal<Function>(&p_call_this_often)->Call(ToLocal<Object>(&p_empty_obj), argc, argv);

}

Unfortunately, I'm not sure there's much we can ultimately do about that.  If you have Persistents that are not weak, am I'm assuming these ones aren't, then the casting is at least safe to do.  It might be good to add a debug ASSERT to your ToLocal function that check that the persistent passed is never weak as well as a AssertNoGCScope around the code that uses a casted Persistent.

Right now they'd be:

- Object::HasIndexedPropertiesInExternalArrayData()

- Object::GetIndexedPropertiesExternalArrayData()

- Object::GetIndexedPropertiesExternalArrayDataLength()

All those functions don't allocate heap memory, so we may be able to provide persistent versions of those if absolutely necessary.  If our fix to the weak callbacks removes the need to have those in v8.h, then we'd like not to add a second version of them to the api.

[Mike Moening]

I've got a simple example in yellow below of using Persistent that is broken with the recent API changes.

Can somebody tell me how to re-write this so that it will compile with the new API changes?

I'm a tad lost on how to make it work...

I'm seeing many errors now:

error C2664: 'v8::Persistent<T>::New' : cannot convert parameter 2 from 'v8::Persistent<T>' to 'v8::Context *'

        with

        [

            T=v8::Context

        ]

        No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called

error C2248: 'v8::Persistent<T>::operator ->' : cannot access private member declared in class 'v8::Persistent<T>'

        with

        [

            T=v8::Context

        ]

        C:\Dev\common\V8\include\v8.h(771) : see declaration of 'v8::Persistent<T>::operator ->'

        with

        [

            T=v8::Context

        ]

error C2664: 'v8::Persistent<T>::New' : cannot convert parameter 2 from 'v8::Local<T>' to 'v8::Object *'

        with

        [

            T=v8::Object

        ]

        No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called

error C2248: 'v8::Persistent<T>::Persistent' : cannot access private member declared in class 'v8::Persistent<T>'

        with

        [

            T=v8::Context

        ]

v8.h(751) : see declaration of 'v8::Persistent<T>::Persistent'

        with

        [

            T=v8::Context

        ]

-------------------------------------------------------------------

The Code:

class ExecState

{

private:

    Persistent<Object>  m_oGlobal;

protected:

    Isolate*            m_pIsolate;

    Persistent<Context> m_oContext;

    

public:

    void SetContext(Isolate* pIsolate, Persistent<Context> oContext)

    {

       m_pIsolate = pIsolate;

       m_oContext  = Persistent<Context>::New(pIsolate, oContext);

       m_oGlobal   = Persistent<Object>::New(pIsolate, oContext->Global());

    }

    Persistent<Context> GetContext() {return m_oContext;}

};

Thank you very much!

Mike M

[Ben Noordhuis]

You replace this:

pers = Persistent<Object>::New(isolate, obj);

With this:

pers.Reset(isolate, obj);

Then, when you want to unwrap the object:

Local<Object> obj = Local<Object>::New(isolate, pers);

You can take a shortcut that avoids the call to Local<T>::New() when
the persistent handle is strong:

Local<Object> obj;
if (pers.IsWeak())
obj = Local<Object>::New(isolate, pers); // same as before
else
obj = *reinterpret_cast<Local<Object>*>(&pers);

Yes, it looks very gnarly.

[Mike Moening]

Thanks for your help.

Something isn't quite right in my head yet.

Can you take my code snippet and re-compile it with your changes (gnarly ones too)?

I think there are more errors yet in there.

Thank you!

[Dan Carney]

Constantly needing to create a Local<T>::New() every time I need to use a Persistent<T> in cases like the above isn't playing nicely. I've noticed since the change that GC time from --prof has grown around 5% in high I/O scenarios.

It really shouldn't change GC times whatsoever.  I did just land a patch that might fix this, but I have no idea if it addresses the issue you're seeing here.

[Ben Noordhuis]

The issue Trevor's reporting is that each Local that's created with
Local<T>::New() gets added to the active HandleScope's internal list.
Before the Persistent changes, dereferencing a Persistent was cheap,
now it's not (if it's weak.)

Oh well, ç'est la vie. I agree with the change. By and large, it's
for the better IMO.

[Dan Carney]

The issue Trevor's reporting is that each Local that's created with
Local<T>::New() gets added to the active HandleScope's internal list.
Before the Persistent changes, dereferencing a Persistent was cheap,
now it's not (if it's weak.)

That still shouldn't affect GC times very much, which is the problem Trevor mentioned, unless the Local::New calls are in the weak callbacks themselves and being counted with GC time.  My patch attempts to fix an issue where handles assigned during weak callbacks were badly scoped, so it might improve GC times, but it also might not help at all if you don't happen to hit it.

[Jim Acquavella]

Why can't the Persistent use a RIAA pattern that's reference counted?  Ideally, I'd like to create a persistent, copy it to another persistent (which would increment the reference), then the last one to go out of scope should be disposed automatically in the destructor.   In other words, I'd like to use it like a boost::shared_ptr.

http://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization

This would tremendously simplify my code and ownership of these persistents.

Thoughts?

[Mike Moening]

I think that's a GREAT idea!
I was planning on sticking my Persistent in a ref counted object.
Why not add this to Persistent instead?

Seems like this idea solves lots of problems.
How many does it create?

[Isaac Burns]

I have used the following wrapper template to manage assigning new values to a persistent handle.

The constructors are only for clarity.  The real work is done in the destructor and assignment operator.

It seems to work, but I welcome any corrections or feedback.

template <class T> class SafePersistent : public v8::Persistent<T>

{

public:

SafePersistent<T>() : v8::Persistent<T>() {}

SafePersistent<T>(v8::Handle<T> handle) : v8::Persistent<T>(handle) {}

~SafePersistent<T>() { CheckDispose(); }

v8::Persistent<T>& operator=(const v8::Persistent<T>& other)

{

CheckDispose();

return v8::Persistent<T>::operator=(other);

}

protected:

void CheckDispose()

{

if (!v8::Persistent<T>::IsEmpty())

{

v8::Persistent<T>::Dispose();

v8::Persistent<T>::Clear();

}

}

};

[Jim Acquavella]

But it's not reference counted and copyable. I want the last reference to dispose the handle. 

Sent from my iPad

--

[Dan Carney]

On Friday, August 23, 2013 12:12:31 AM UTC+2, Jim Acquavella wrote:

Why can't the Persistent use a RIAA pattern that's reference counted?  Ideally, I'd like to create a persistent, copy it to another persistent (which would increment the reference), then the last one to go out of scope should be disposed automatically in the destructor.   In other words, I'd like to use it like a boost::shared_ptr.

This is one of the ideas that we've discussed internally.  The problem is that persistent carries a lot of state with it, and that state is not generally amenable to reference counting.  A weak handle that is reference counted is particularly problematic, for instance, since most weak callbacks dispose the handle, leaving any references to it invalid.  Unless you can find all copies of the handle out there, you've got a use after free bug waiting for you.

It's much better to write a wrapper class for refcounted persistents if your application demands it.

[Jim Acquavella]

But if you centralize the storage and information associated with an indirection, then all copies, weak or otherwise should point to the same object.  If during a weak callback disposes the handle, and others are referring to it such that it isn't the last reference, the memory associated with the handle shouldn't be disposed, but the handle data should be.  Then, the other handles when calling IsEmpty should return true.  The centralized data used for the Persistent is reference counted and only when the reference count becomes zero should the actual centralized storage memory be disposed.  Make sense?

[Dan Carney]

On Tuesday, August 27, 2013 1:53:03 AM UTC+2, Jim Acquavella wrote:

But if you centralize the storage and information associated with an indirection, then all copies, weak or otherwise should point to the same object.  If during a weak callback disposes the handle, and others are referring to it such that it isn't the last reference, the memory associated with the handle shouldn't be disposed, but the handle data should be.  Then, the other handles when calling IsEmpty should return true.  The centralized data used for the Persistent is reference counted and only when the reference count becomes zero should the actual centralized storage memory be disposed.  Make sense?

It makes sense technically, yes, but it makes the api semantically confusing, which we'd like to avoid.  There are certain actions which would force the duplication of the storage cell - like calling make weak on a cell that's already weak.  Or even calling SetWrapperClassId.  A persistent can't have 2 weak callbacks or 2 class_ids, so duplicating the cell is the only logical thing to do, and duplicating the cell is very unnatural in the context of reference counting.

To do correctly, it would essentially require some sort of persistent builder or something which would ensure that it can't be modified after creation (except in the weak callback).  As absurd as may sound, I'd like to minimize the changes to the api.

Additionally, handles are meant to be strong, and passing around a persistent that might just disappear outside of its weak callback is somewhat contrary to the way the api is intended to work.

[Mike Moening]

Honestly right now persistent is so hard to use his suggestion would be welcome. Making the end user build his own ref counting scheme around Persistent is not better IMO.

[Dan Carney]

Just a heads up.  All old style callbacks will be gone in the next release.  They have been deprecated for some time, but now anything using v8::Arguments or v8::AccessorInfo won't compile.

Just a reminder, to convert a function like:

Handle<Value> OldCallback(const v8::Arguments& args) {

  Local<Value> x = ... // do stuff with args

  return x;

}

you just simply do this:

void NewCallback(const v8::FunctionCallbackInfo& info) {

  Local<Value> x = ... // do stuff with info

  info.GetReturnValue().Set(x);

}

I'll update the wrapper library at https://github.com/drcarney66/v8-backport to reflect the changes.  Should people have a lot of functions to convert, it might be easier to use that or something like it.  Any callback needing to be fast probably wants to use new style callbacks.

[Jim Acquavella]

Is calling MakeWeak on an already weak handle something that needs to be supported?  I'd say you add a clone method and have the items that force storage cell duplication return an error.  Supporting common use case that makes these easy to use should be the goal.  

[Isaac Burns]

I see.  You want to make the handle reference counted instead of the pointer it manages.

I need to look a little more closely at the v8 handle code.  I was under the assumption that v8::Handle was the base class for v8::Local and v8::Persistent.  Where v8::Local would dispose of everything inside a v8::HandleScope and v8::Persistent is only disposed manually.

With this, I can create a v8::Persistent where I can assign new v8::Values, provided I check and dispose of any current value first.  Also, any class that has a v8::Persistent as a member, must check and dispose of it when destructed.  The wrapper template handles these cases automatically.

[ioannis]

So with the resent changes to Persistent how can we store a series of Persistent<Function> handles to an STL container like map ?

[Jim Acquavella]

I started using this:

template <typename T> struct PersistentP : public boost::shared_ptr<v8::Persistent<T> >

{

typedef boost::shared_ptr<v8::Persistent<T> > _inherited;

PersistentP(v8::Persistent<T> *inPersistentP0 = NULL) {

_inherited::reset(inPersistentP0);

}

v8::Local<T> GetLocal() const {

return v8::Local<T>::New(v8::Isolate::GetCurrent(), *_inherited::get());

}

};

This indirection greatly simplifies my code!

On Mon, Sep 16, 2013 at 3:18 PM, ioannis <ioan...@gmail.com> wrote:

So with the resent changes to Persistent how can we store a series of Persistent<Function> handles to an STL container like map ?

--

[Dan Carney]

On Tuesday, September 17, 2013 12:18:36 AM UTC+2, ioannis wrote:

So with the resent changes to Persistent how can we store a series of Persistent<Function> handles to an STL container like map ?

You definitely want some kind of wrapper, depending on usage, and Jim's suggestion is an excellent example.

We're planning to add a UniquePersistent class which has semantics like unique_ptr to allow use in std collections.

[Dan Carney]

The next round of changes to Persistent are in.  V8_USE_UNSAFE_HANDLES no longer exists.  Persistent now has a second template parameter that controls copy and assignment behaviour.  The default parameters keep the behaviour unchanged.  To allow copying one could use such a class as the below 

template<class T>

struct CopyablePersistentTraits {

  typedef Persistent<T, CopyablePersistentTraits<T> > CopyablePersistent;

  static const bool kResetInDestructor = true;

  template<class S, class M>

  static void Copy(const Persistent<S, M>& source, CopyablePersistent* dest) {}

};

the Copy function is called on both copy and assign, and the kResetInDestructor variable controls whether Reset is called in the destructor.

Setting kResetInDestructor = true is highly recommended, and we plan on changing the default traits class at some point to do this.

We will keep the default functionality of disallowing copying and assignment as is, as these are expensive operations.  An embedder should only perform them intentionally.

[Luke]

I'm in the process of integrating V8 into Go. Go does not understand C++, so I've been having to cast Persistent handles to void*. I'm assuming this is the only way to do it. The void* itself is wrapped in a C struct with an enum specifying the type.

In one particular case a Persistent handle (wrapped) is passed into a function, and then the function passing that parameter disposes the Persistent handle. If the handle is a Function, then it needs a copy of that Persistent handle. Is this what I have to do to copy the Persistent handle, or is there a better way?

Luke