Bug 143181

Also see the bug 142842: The prototype internal slot of an ES6 class should be non-configurable and non-writable See dependency tree for bug 142842. We can either add an extra argument to op_put_by_id or add a new op code to modify the descriptor of a property. Any preferences?

<rdar://problem/20337023>

Looks like defineProperty is mostly used to define a new accessor in benchmarks (run-jsc-benchmarks): The total number of calls to defineOwnIndexProperty: 8 The total number of calls to defineOwnNonIndexProperty: 3562 was a new entry: 3398 didn't change descriptor: 0 configurable: 2224 enumerable: 2168 writable: 1726 accessor: 1726

In Speedometer, it's mostly used to define non-enumerable and non-writable proeprties: The total number of calls to defineOwnIndexProperty: 22 The total number of calls to defineOwnNonIndexProperty: 103034 was a new entry: 66903 (65%) didn't change descriptor: 0 configurable: 73953 (72%) enumerable: 2168 (2%) writable: 1726 (2%) accessor: 1726 (2%)

*** Bug 143246 has been marked as a duplicate of this bug. ***

I talked with Geoff today about this bug, and he suggested that we can just make a function call to defineProperty. We can then add an intrinsic to defineProperty so that it'll go fast in DFG. That'll work nicely with the bug 143239.

Created attachment 249911 [details] WIP Some real craziness.

Created attachment 250046 [details] WIP2

Created attachment 250598 [details] Fixes the bug

Created attachment 251202 [details] Updated for ToT

Comment on attachment 251202 [details] Updated for ToT View in context: https://bugs.webkit.org/attachment.cgi?id=251202&action=review > Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:572 > + unsigned m_linkTimeConstants[LinkTimeConstantCount] { 0 }; Looks like Windows can’t handle this.

Comment on attachment 251202 [details] Updated for ToT View in context: https://bugs.webkit.org/attachment.cgi?id=251202&action=review >> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:572 >> + unsigned m_linkTimeConstants[LinkTimeConstantCount] { 0 }; > > Looks like Windows can’t handle this. I'd swear we've used things like this before -- you could try making LinkTimeConstantCount static or a class member. Also honestly i'd prefer it if you used std::array here sit has bounds checking at least in debug builds (alas not in release :-/ )

Created attachment 251369 [details] Fixed Windows build

Created attachment 251404 [details] Another attempt to fix Windows build

Created attachment 251425 [details] Fixed Windows build for good; fuck off cl.exe

Created attachment 251468 [details] Yet another Win build fix; fuck cl.exe

Fucking cl.exe needs to die.

Comment on attachment 251468 [details] Yet another Win build fix; fuck cl.exe View in context: https://bugs.webkit.org/attachment.cgi?id=251468&action=review Patch looks OK to me. Please address the comments below, and fix the build. > Source/JavaScriptCore/bytecode/CodeBlock.cpp:1769 > + for (unsigned i = 0; i < LinkTimeConstantCount; i++) { > + LinkTimeConstant type = static_cast<LinkTimeConstant>(i); > + if (unsigned registerIndex = unlinkedCodeBlock->registerIndexForLinkTimeConstant(type)) > + m_constantRegisters[registerIndex].set(*m_vm, ownerExecutable, m_globalObject->jsCellForLinkTimeConstant(type)); > + } Can we unify LinkTimeConstant with SpecialPointer? They implement the same feature, so it would be nice to unify them. SpecialPointer is just a LinkTimeConstant for one of two possible functions (call, apply), and LinkTimeConstant adds another (defineProperty). > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:152 > + m_linkTimeConstantRegisters = { {nullptr} }; Will this syntax fill all entries or just the first? We want to fill all.

(In reply to comment #18) > Comment on attachment 251468 [details] > Yet another Win build fix; fuck cl.exe > > View in context: > https://bugs.webkit.org/attachment.cgi?id=251468&action=review > > Patch looks OK to me. Please address the comments below, and fix the build. > > > Source/JavaScriptCore/bytecode/CodeBlock.cpp:1769 > > + for (unsigned i = 0; i < LinkTimeConstantCount; i++) { > > + LinkTimeConstant type = static_cast<LinkTimeConstant>(i); > > + if (unsigned registerIndex = unlinkedCodeBlock->registerIndexForLinkTimeConstant(type)) > > + m_constantRegisters[registerIndex].set(*m_vm, ownerExecutable, m_globalObject->jsCellForLinkTimeConstant(type)); > > + } > > Can we unify LinkTimeConstant with SpecialPointer? They implement the same > feature, so it would be nice to unify them. SpecialPointer is just a > LinkTimeConstant for one of two possible functions (call, apply), and > LinkTimeConstant adds another (defineProperty). Yeah, that's the plan but I didn't want to do that in this patch since it's already quite massive. > > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:152 > > + m_linkTimeConstantRegisters = { {nullptr} }; > > Will this syntax fill all entries or just the first? We want to fill all. It should initiailize them all but I'm gonna just use loop since it doesn't seem like nothing compiles these days.

Created attachment 251514 [details] Fixed builds

Created attachment 251521 [details] Another build fix

Comment on attachment 251521 [details] Another build fix View in context: https://bugs.webkit.org/attachment.cgi?id=251521&action=review > Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:334 > + unsigned addConstant(LinkTimeConstant type) ASSERT it’s < LinkTimeConstantCount? > Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:343 > + unsigned registerIndexForLinkTimeConstant(LinkTimeConstant type) ASSERT it’s < LinkTimeConstantCount? > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:153 > + for (unsigned i = 0; i < LinkTimeConstantCount; i++) > + m_linkTimeConstantRegisters[i] = nullptr; Can this use a modern for loop instead? > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:186 > + for (unsigned i = 0; i < LinkTimeConstantCount; i++) > + m_linkTimeConstantRegisters[i] = nullptr; Can this use a modern for loop instead? > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:504 > + for (unsigned i = 0; i < LinkTimeConstantCount; i++) > + m_linkTimeConstantRegisters[i] = nullptr; Can this use a modern for loop instead?

(In reply to comment #22) > Comment on attachment 251521 [details] > Another build fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=251521&action=review > > > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:153 > > + for (unsigned i = 0; i < LinkTimeConstantCount; i++) > > + m_linkTimeConstantRegisters[i] = nullptr; > > Can this use a modern for loop instead? How do I do that?

Okay, we can just do: for (auto& constantRegister : m_linkTimeConstantRegisters) constantRegister = nullptr;

Thanks for the review! Will land the patch after using modern loops and adding assertions.

Committed r183316: <http://trac.webkit.org/changeset/183316>