[webkit-reviews] review requested: [Bug 40060] Add a version of JSObjectSetPrototype that yields an exception when the prototype can't be set : [Attachment 59259] Fix v1.1

[bugzilla-daemon at webkit.org]

Jędrzej Nowacki <jedrzej.nowacki at nokia.com> has asked  for review:
Bug 40060: Add a version of JSObjectSetPrototype that yields an exception when
the prototype can't be set
https://bugs.webkit.org/show_bug.cgi?id=40060

Attachment 59259: Fix v1.1
https://bugs.webkit.org/attachment.cgi?id=59259&action=review

------- Additional Comments from Jędrzej Nowacki <jedrzej.nowacki at nokia.com>
(In reply to comment #6)
> > > > +void JSObjectSetPrototypeEx(JSContextRef ctx, JSObjectRef object,
JSValueRef value, JSValueRef* exception)
> > >
> > > You need to discuss the naming of this with Geoff Garen. The "Ex" suffix
is almost certainly *not* what we want to do here.
>
> I agree that the "Ex" naming isn't good. It doesn't say what's different
about the "Ex" version, and it only works once. If we make another mistake,
will we name the next function "ExEx?"
Oh, then we will use "ExBis" :-) I do know that "Ex" is wrong but which name
would be right? What about JSObjectSetPrototypeWithErrorHandling? I'm attaching
rebased patch.

> > > I’m not sure the C API needs this feature. What’s the reason we need to
add this? Is someone asking for it?
> > Yes, I'm asking for it :-).
> I'm not convinced the C API needs this feature, either.
> (...)
> However, to have two versions of the same function, I think the second
version really needs to justify itself.
> (...)

This function can stay in a private header for now, for example next version of
JSC API and then it could replace the original one. I don't insist to create an
officially supported API, I'm fine with binary compatibility breakages or even
small refactoring as long as I can access to the functionality with good
performance. Is API's performance a good justification to introduce the
function?

I wrote 5 implementation of the QScriptValue::setPrototype(). I test them using
the same benchmark:

  QScriptEngine engine;
  QScriptValue value = engine.newObject();
  QScriptValue proto1 = engine.newObject();
  QScriptValue proto2 = engine.newObject();
  QScriptValue tmp = engine.newObject();
  tmp.setPrototype(engine.newObject());
  proto2.setPrototype(tmp);

  QBENCHMARK {
      value.setPrototype(proto1);
      value.setPrototype(proto2);
  }

The benchmark simply call the setPrototype function changing a prototype for
the value variable,
proto1 represent a short prototype chain (0 element above proto1),
proto2 represent a longer prototype chain (2 elements above proto2).
QBENCHMARK macro is a part of Qt benchmarking tool.

#if 1
// This solution use a new API.

	JSValueRef exception = 0;
	JSObjectSetPrototypeWithErrorHandling(*m_engine, *this, *prototype,
&exception);
	if (exception)
	    qWarning("QScriptValue::setPrototype() failed: cyclic prototype
value");

//RESULT : tst_QScriptValue::setProtoBench():
//     0.00047 msecs per iteration (total: 62, iterations: 131072)

#elif 0

// This solution tries to set the __proto__ property

	JSValueRef exception = 0;
	JSRetainPtr<JSStringRef> protoName =
JSStringCreateWithUTF8CString("__proto__");
	JSObjectSetProperty(*m_engine, *this, protoName.get(), *prototype, /*
attr */ 0, &exception);
	if (exception)
	    qWarning("QScriptValue::setPrototype() failed: cyclic prototype
value");
	    
//RESULT : tst_QScriptValue::setProtoBench():
//     0.0012 msecs per iteration (total: 80, iterations: 65536)
     
#elif 0

// This solution iterates over value's prototype chain to check if an error can
occur.

	JSValueRef proto = *prototype;
	while (!JSValueIsNull(*m_engine, proto)) {
	    if (JSValueIsStrictEqual(*m_engine, proto, *this)) {
		qWarning("QScriptValue::setPrototype() failed: cyclic prototype
value");
		return;
	    }
	    JSValueRef exception = 0;
	    proto = JSObjectGetPrototype(*m_engine, JSValueToObject(*m_engine,
proto, &exception));
	    if (exception) {
		qWarning("OOPS");
		return;
	    }
	}
	JSObjectSetPrototype(*m_engine, *this, *prototype);

//RESULT : tst_QScriptValue::setProtoBench():
//     0.0036 msecs per iteration (total: 60, iterations: 16384)

#elif 0

//This solution is same as previous one, but it use a trick to avoid redundant
conversion from
//JSValueRef to JSObjectRef.

	JSValueRef proto = *prototype;
	while (!JSValueIsNull(*m_engine, proto)) {
	    if (JSValueIsStrictEqual(*m_engine, proto, *this)) {
		qWarning("QScriptValue::setPrototype() failed: cyclic prototype
value");
		return;
	    }
	    proto = JSObjectGetPrototype(*m_engine,
const_cast<JSObjectRef>(proto));
	}
	JSObjectSetPrototype(*m_engine, *this, *prototype);

//RESULT : tst_QScriptValue::setProtoBench():
//     0.0028 msecs per iteration (total: 92, iterations: 32768)
     
#else

//This solution is based on implementation detail, I know that only possible
error is the "cyclic prototype value",
//so if the JSObjectSetPrototype fails to set a prototype then I know what
happened.

	JSObjectSetPrototype(*m_engine, *this, *prototype);
	JSValueRef proto = JSObjectGetPrototype(*m_engine, *this);
	if (!JSValueIsStrictEqual(*m_engine, proto, *prototype)) {
	    qWarning("QScriptValue::setPrototype() failed: cyclic prototype
value");
	}

//RESULT : tst_QScriptValue::setProtoBench():
//     0.00091 msecs per iteration (total: 60, iterations: 65536)

#endif


The first solution is the shortest and the fastest one (2 - 8x faster then
others).