Adding support for tagged values with JSRT weak reference APIs

[MSLaguana]

Tagged values will never be GCd, and don't have correpsonding heap
blocks. Since tagged values are an internal detail, we should handle
it at the JSRT layer, and since tagged values won't be GCd there is
no difference between a strong reference and a weak reference.

Should fix nodejs/node-chakracore#380

Alternative to #3591

[MSLaguana]

I was considering making this check be !Js::RecyclableObject::Is(value) instead, but I wasn't sure how to validate the incoming weak reference in JsGetWeakReferenceValue. Are weak reference pointers guaranteed to have any particular shape?

[jianchun]

Reading the code, looks weak reference pointer is the same normal pointer as other pointers. Why do you consider that alternative? It looks to me TaggedNumber::Is works fine and probably better. Used elsewhere as well.

nit: Consider moving this fragment up out of GlobalAPIWrapper. It is simple operation and no need to enter the wrapper.

[MSLaguana]

I was considering checking for RecyclableObject since that would be potentially more future-proof in case we add non-number non-recyclable objects.

I'll move the code out of the wrapper as you suggest.

[curtisman]

I wonder whether it would be better to return nothing. There may be scenario that the "user" of the API might need to know if it need to keep track of the weak reference or not (like keeping it in a list for caching, for example)? With this, it will not be able to identify whether it needs to or not?

[MSLaguana]

In that case, would you prefer to return an error code like JsNoWeakRefRequired or some other descriptive name?

I feel like that might be pushing more implementation details onto the embedder/consumer of chakracore. The scenario where this came up for node-chakracore was in an API that node exposes to native module implementers: the API contract there is "given a JS object, create a reference to it with N references (if N is 0 then create a weak reference)". If we modified the JSRT so that it errored out / returned a nullptr for non-GC-able objects, we'd still need to implement essentially the same fakery in the node-chakracore napi implementation as I'm doing here.

[digitalinfinity]

Alternatively, we could return a fake weak reference too. @curtisman I was wondering the same thing and I landed at the same conclusion as @MSLaguana which is, for unboxed values, it's probably better to return the value back since it's an opaque handle anyway and the caller wouldn't need to do any special processing of the returned result or be careful of what parameters the API is called with. Yet another alternative would be to ensure that JSRT always returns boxed values but I think that's too high a performance price to pay.

[curtisman]

I prefer returning an error code. Yes, we are pushing some implementation details on the embedder, but at least it is doing the "right" thing. The caller can decide what to do with it, (for your case, use the var itself or for the cache case, discard and not cache it) The API will force the caller to deal with it correctly, instead of having some default behavior that might cause other issues.

[MSLaguana]

How do you propose that an embedder would handle the situation I described above? Something like?

void create_weak_ref(JsValueRef val, EmbedderRefT * ref){
  *ref = new EmbedderRefT();
  JsWeakRef jsrtWeak = nullptr;
  auto err = JsCreateWeakReference(val, &jsrtWeak);
  if (err == JsNoWeakRefNeeded)
  {
    ref->realWeak = false;
    ref->value = val;
  }
  else
  {
    ref->realWeak = true;
    ref->value = jsrtWeak
  }
  return;
}

void deref_weak_ref(EmbedderRefT ref, JsValueRef *val) {
  if (ref->realWeak)
  {
    JsGetWeakReferenceValue(ref->value, val);
  }
  else
  {
    *val = ref->value;
  }
}

[curtisman]

Unfortunately, yes.

My main concern with "just return the tagged value as a weak reference" is that JsGetWeakReferenceValue will never return NULL for one for the tagged value, since it will never need to be collected, which breaks semantic of what a weak reference behaves.

I am curious what v8's API does with this case.

[MSLaguana]

I had a poke around at the v8 implementation of the relevant napi function, and it looks like they may force values to be references. They construct a v8::Persistent from the given value, and that ends up going down into v8::GlobalizeReference which dereferences the value.

I'm not sure how tagged values fit into that, I didn't see any special handling for it.

[MSLaguana]

Regarding your concern about the weak references never becoming null: Isn't the same potentially true of other real weak references? E.g. if I create a weak reference to the global object, or to Symbol.toStringTag, or some other spec-defined object, all of those will have legitimate weak references to an object that should never be garbage collected.

[curtisman]

All GC object can get collected when there are no more reference to it. That includes the global object when the context get collected.

A tagged value doesn't because it is not a GC object

[MSLaguana]

@curtisman, @digitalinfinity and I have been discussing this and I'd still like to support weak references for tagged values, since there is currently no way for a host to distinguish tagged values and regular values at the moment, and we can provide accurate weak-ref semantics (in terms of "here is a handle which you can give back and either get the original object, or null if it is gone").

[curtisman]

Why returning JsNoWeakRefRequired is not sufficient?

I am not exactly happy to "fake" the weak reference, however given how Node use it, I think they will end up using the tagged value as the weak reference value anyway.

I guess if we do it this way, if the user really need to know if the weak reference is really a weak reference (and will ever be collected or not), they can compare the weak ref value to the actual value. Although it is a pretty round about way. I just think it is cleaner for the API to be clear about what actually happen (weak reference not being created).

With all that said, I am not going block this going forward.

[MSLaguana]

It looks like node has changed to no longer support/require weak refs to non-objects, so my main objection isn't so relevant any more. As such, I've now opened up #3768 which reports an error with non-RecyclableObject weak refs.