Bug 143181 – class methods should be non-enumerable

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Ryosuke Niwa]

Both instance and static methods defined by ES6 class syntax should not be enumerable.

[Ryosuke Niwa]

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?

[Radar WebKit Bug Importer]

<rdar://problem/20337023>

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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%)

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 249911 [details]
WIP

Some real craziness.

[Ryosuke Niwa]

Created attachment 250046 [details]
WIP2

[Ryosuke Niwa]

Created attachment 250598 [details]
Fixes the bug

[Ryosuke Niwa]

Created attachment 251202 [details]
Updated for ToT

[Darin Adler]

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.

[Oliver Hunt]

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 :-/ )

[Ryosuke Niwa]

Created attachment 251369 [details]
Fixed Windows build

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Fucking cl.exe needs to die.

[Geoffrey Garen]

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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Created attachment 251514 [details]
Fixed builds

[Ryosuke Niwa]

Created attachment 251521 [details]
Another build fix

[Darin Adler]

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?

[Ryosuke Niwa]

(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?

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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