A try to create cycle in a prototype chain will end up in an exception > a = new Object [object Object] > a.__proto__=a Exception: Error: cyclic __proto__ value > This is not true in the JSC C API. Invalid call to the JSObjectSetPrototype will be just ignored. It would be nice to have an unified interface here. I think that the JSObjectSetPrototype function should take one parameter more (an exception handler), but as it is not possible without breaking binary compatibility, a new function should be created. This bug is a next step for bug 39360.
Created attachment 57654 [details] Fix v1
Comment on attachment 57654 [details] Fix v1 > +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.
Comment on attachment 57654 [details] Fix v1 I’m not sure the C API needs this feature. What’s the reason we need to add this? Is someone asking for it?
Comment on attachment 57654 [details] Fix v1 r- per above naming comment.
(In reply to comment #2) > (From update of attachment 57654 [details]) > > +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. It is why, he is in CC :-) I don't know Mac naming policy, I agree that the name could be better. In C++ I could use function overloading, but it is a C code. Do you have idea for a better name? (In reply to comment #3) > (From update of attachment 57654 [details]) > 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 :-). 1. I think there is an inconsistency; in the jsc console creating cycle in a prototype chain results in an exception in C API these exception is ignored. 2. I would like to implement fix for bug 39356, current QtScript implementation shows a warning when an user tries to create a cyclic prototype chain.
> > > +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?" > > 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. Bug 39360 was clearly an improvement -- it's nice when the API can prevent you from shooting yourself in the foot without undue performance cost. However, to have two versions of the same function, I think the second version really needs to justify itself. I believe you can get the behavior you want for bug 39356 using the current API, by either: (a) setting the "__proto__" property using JSObjectSetProperty. or (b) using JSObjectGetPrototype in a loop to check for cycles yourself.
Created attachment 59259 [details] Fix v1.1 (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).
> > 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? Something that denoted the difference between the two functions. > What about JSObjectSetPrototypeWithErrorHandling? JSObjectSetPrototypeWithException would probably be better. The function doesn't actually handle errors -- it just supports an exception out parameter. > 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 think it's practical to replace the old function, since that would break binary compatibility. > Is API's performance a good justification to introduce the function? It can be. It depends on whether this is a performance-critical path. Is it? If so, why?
Attachment 59259 [details] did not build on win: Build output: http://webkit-commit-queue.appspot.com/results/3299550
Created attachment 59358 [details] Fix v1.2 (In reply to comment #8) > JSObjectSetPrototypeWithException would probably be better. The function doesn't actually handle errors -- it just supports an exception out parameter. Thanks, changed. > > 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 think it's practical to replace the old function, since that would break binary compatibility. You mean that we won't be able to break the binary compatibility promise, never? > > Is API's performance a good justification to introduce the function? > It can be. It depends on whether this is a performance-critical path. Is it? If so, why? As the QtScript is an API, I have a similar problem, like the JSC C API, to really define a good use cases. Lets try a logical experiment. Could you give me a performance-critical path for setting JSObjectSetProperty? If yes then it is the same as mine :-) if not can I put in the implementation a sleep() call? I try to make implementation of the QtScript clean and fast as possible. As you pointed me, there are a few ways to get the behavior, but I believe that, they are not so clean and fast as they could be. If you could create the JSObjectSetPrototype function again, assuming that you would knew about the exception, would you create it with an exception parameter or without? I think, it should be fixed, but I don't want to push to much as I can live without it (2x time longer as it is as much slower :-) ).
Attachment 59358 [details] did not build on win: Build output: http://webkit-commit-queue.appspot.com/results/3325568
(In reply to comment #11) > Attachment 59358 [details] did not build on win: > Build output: http://webkit-commit-queue.appspot.com/results/3325568 How to download the build log ?
Geoff, I think you should make the call on this. The mechanics of the patch look OK to me.
> > I don't think it's practical to replace the old function, since that would break binary compatibility. > You mean that we won't be able to break the binary compatibility promise, never? Correct. That's one reason we're particularly conservative about API. > Lets try a logical experiment. Could you give me a performance-critical path for setting JSObjectSetProperty? If yes then it is the same as mine :-) if not can I put in the implementation a sleep() call? This experiment didn't persuade me, since there's no clear benefit to adding a sleep() call to JSObjectSetProperty(). > If you could create the JSObjectSetPrototype function again, assuming that you would knew about the exception, would you create it with an exception parameter or without? If I had it all to do over again, I would either add the exception parameter or remove all exception parameters and make pending exceptions an implicit property of the execution context.
Comment on attachment 59358 [details] Fix v1.2 I don't think we ever want to make JSObjectSetPrototypeWithException public API. However, I don't see any downside to adding it as private API -- it doesn't introduce any new dependencies on the VM. So, let's compromise by adding this as private API, which Qt library implementors can use, with the understanding that we're not going to make it into public API.
Comment on attachment 59358 [details] Fix v1.2 Rejecting patch 59358 from commit-queue. Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1 Last 500 characters of output: ompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/JavaScriptCore.build/Debug/jsc.build/Objects-normal/i386/jsc.o /Users/eseidel/Projects/CommitQueue/JavaScriptCore/jsc.cpp normal i386 c++ com.apple.compilers.gcc.4_2 testapi: Distributed-CompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/JavaScriptCore.build/Debug/testapi.build/Objects-normal/i386/testapi.o /Users/eseidel/Projects/CommitQueue/JavaScriptCore/API/tests/testapi.c normal i386 c com.apple.compilers.gcc.4_2 (68 failures) Full output: http://queues.webkit.org/results/3923394
Comment on attachment 59358 [details] Fix v1.2 It seems this would break Mac, based on the commit-queue failure.