Bug 141106 – Implement ES6 Object.getOwnPropertySymbols

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

Implement ES6 Object.getOwnPropertySymbols

[Yusuke Suzuki]

One issue is that, it can expose property symbols of the target object.
Currently, JSC priviledge code uses *private* symbols. For example, in builtin/, @Object is used to extract builtin Object. Object.getOwnPropertySymbols(window) should not expose these private Symbols.

So, here's a roadmap.

1. Introduce a new flag to StringImpl*, which represents the target Symbol is truly private Symbol. (Maybe, isPrivate).
2. Rename JSObject::getOwnPropertyNames to JSObject::getOwnPropertyKeys to align to the ES6 spec, and accept EnumerationType parameter (String or Symbol).
3. May rename getPropertyNames to getPropertyKeys, Name is now used for representing String property in the ES6 spec.
4. Implement Object.getOwnPropertySymbols by leveraging this JSObject::getOwnPropertySymbols.

[Yusuke Suzuki]

Created attachment 246164 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 246164 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=246164&action=review

Added comments.

> Source/JavaScriptCore/ChangeLog:18
> +        This conservative assumption doesn't break the callers.

Currently we doesn't rename `getOwnPropertyNames` to `getOwnPropertyKey` which is used in the ES6 draft spec.
If it's preferrable, I'll rename it.

[Yusuke Suzuki]

Created attachment 246199 [details]
Patch

[Yusuke Suzuki]

Created attachment 246213 [details]
Patch

[Yusuke Suzuki]

Created attachment 246215 [details]
Patch

[Darin Adler]

Gavin might be a good reviewer for this.

[Darin Adler]

Comment on attachment 246215 [details]
Patch

Is adding a bit to StringImpl the best way to distinguish these? It seems a shame to have to build a feature into WTF just for this. It’s OK if there’s no other efficient way.

[Yusuke Suzuki]

(In reply to comment #8)
> Comment on attachment 246215 [details]
> Patch
> 
> Is adding a bit to StringImpl the best way to distinguish these? It seems a
> shame to have to build a feature into WTF just for this. It’s OK if there’s
> no other efficient way.

Thank you for adding reviewers and commenting on the patch.

StringImpl pointer is now used in PropertyTable directly.
So adding a status to StringImpl is the most efficient way. And if we add the flag to PropertyTable Key, the memory usage of PropertyTable is doubled. Or if we create some structure holding StringImpl* and status and strore it to PropertyTable, we need to refer StringImpl indirectly and additional heap allocation is needed.

One possible solution is storing a flag to StringImpl*'s lower bit in PropertyTable (due to alignment, some space is reserved), but it seems that it's a little bit hacky.

[Darin Adler]

Comment on attachment 246215 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=246215&action=review

Still not comfortable reviewing the whole thing, but I have a few more small comments.

> Source/JavaScriptCore/runtime/EnumerationMode.h:38
> +enum class EnumerationType {

I think we should combine both EnumerationMode and EnumerationType into a single integer with 3 orthogonal bits.

> Source/JavaScriptCore/runtime/EnumerationMode.h:40
> +    IncludeSymbol,
> +    ExcludeSymbol

Should be Include/ExcludeSymbols, plural.

> Source/WTF/wtf/text/StringImpl.h:386
> +    ALWAYS_INLINE static Ref<StringImpl> createUniquePrivate()

I think this still needs the name empty in it, to specify that the unique private string will have a value of “empty string”.

> Source/WTF/wtf/text/StringImpl.h:403
>      static unsigned flagIsAtomic() { return s_hashFlagIsAtomic; }
>      static unsigned flagIsUnique() { return s_hashFlagIsUnique; }
> +    static unsigned flagIsPrivate() { return s_hashFlagIsPrivate; }

It’s worth noting that these three flags are not orthogonal. Atomic and Unique are mutually exclusive. Private can only be set if Unique is also set. So I think there are only 4 states here even though we are using 3 bits: 1) non-atomic, non-unique, non-private 2) atomic, 3) unique, 4) unique private.

[Yusuke Suzuki]

Comment on attachment 246215 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=246215&action=review

Thank you for your comment!

>> Source/JavaScriptCore/runtime/EnumerationMode.h:38
>> +enum class EnumerationType {
> 
> I think we should combine both EnumerationMode and EnumerationType into a single integer with 3 orthogonal bits.

Ah, sounds nice.
What do you think introducing `class EnumerationMode` that wraps `unsigned flags` and provides `shouldSkipJSObject()`, `shouldIncludeSymbols()` and `shouldIncludeDontEnumProperties()` member functions?
I think it leverages type checks and cleans up predicate functions.

>> Source/JavaScriptCore/runtime/EnumerationMode.h:40
>> +    ExcludeSymbol
> 
> Should be Include/ExcludeSymbols, plural.

Thanks! I'll fix it.

>> Source/WTF/wtf/text/StringImpl.h:386
>> +    ALWAYS_INLINE static Ref<StringImpl> createUniquePrivate()
> 
> I think this still needs the name empty in it, to specify that the unique private string will have a value of “empty string”.

I'll fix it.

>> Source/WTF/wtf/text/StringImpl.h:403
>> +    static unsigned flagIsPrivate() { return s_hashFlagIsPrivate; }
> 
> It’s worth noting that these three flags are not orthogonal. Atomic and Unique are mutually exclusive. Private can only be set if Unique is also set. So I think there are only 4 states here even though we are using 3 bits: 1) non-atomic, non-unique, non-private 2) atomic, 3) unique, 4) unique private.

Sounds nice.
So I'll introduce 2-bit space to flags instead of 3-bit. And folding above 4 types here.

[Yusuke Suzuki]

Comment on attachment 246215 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=246215&action=review

> Source/WTF/wtf/text/StringImpl.h:797
>      static const unsigned s_hashFlagIsAtomic = 1u << 4;

And seeing the current implementation, `1u << 2` case is missing. So we can drop this.

[Yusuke Suzuki]

Created attachment 246384 [details]
Patch

[Yusuke Suzuki]

Created attachment 246719 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 246719 [details]
Patch

Updated the patch, ready for review.

[Yusuke Suzuki]

Comment on attachment 246719 [details]
Patch

This should be behind the runtime flag, SymbolEnabled.
I'll update the revised patch.

[Yusuke Suzuki]

Created attachment 247333 [details]
Patch

[Darin Adler]

Comment on attachment 247333 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247333&action=review

I think we do want this feature, and this patch seems basically right to me.

I worry a little about how wordy the EnumerationMode::IncludeDontEnumPropertiesAndExcludeSymbols becomes with this change. And I also worry a little about the clarity about when we need to use Identifier::from vs. the Identifier constructor directly. With two functions that can take arguments of the same types but do different jobs, I think we need clearer naming than just “constructor” and “from”.

> Source/JavaScriptCore/ChangeLog:104
> +        In the a lot of place, `Identifier(VM*/ExecState*, StringImpl*)` constructor is used.
> +        However, it's ambiguous because `StringImpl*` has 2 different meanings.
> +        1) normal string, it is replacable with `WTFString` and 2) `uid`, which holds `isUnique` information to represent Symbols.
> +        So we dropped `Identifier(VM*/ExecState*, StringImpl*)` and instead, introduced `Identifier::from(VM*/ExecState*, StringImpl*)`.
> +        This function is used for 2) `uid`. So uniqueness of `StringImpl*` is saved.
> +        To just construct `Identifier` from string type, we can use `Identifier(VM*/ExecState*, const WTFString&)` instead.

Having two separate functions seems fine. But having one be the constructor and the other be named “from” does not make the difference between these functions clear at all. We need to name the functions to explain the difference in their behavior.

> Source/JavaScriptCore/JavaScriptCore.order:-940
> -__ZN3JSC9Structure29getPropertyNamesFromStructureERNS_2VMERNS_17PropertyNameArrayENS_15EnumerationModeE

No need to edit order files. These are generated files that need not be updated by hand. If you rebase this patch, feel free to omit the changes to this file.

> Source/JavaScriptCore/runtime/EnumerationMode.h:44
> +    inline EnumerationMode(EnumerationModeRepresentationTag tag)

The inline keyword here is not needed and should be omitted.

> Source/JavaScriptCore/runtime/EnumerationMode.h:49
> +    inline bool shouldIncludeDontEnumProperties() const

The inline keyword here is not needed and should be omitted.

> Source/JavaScriptCore/runtime/EnumerationMode.h:54
> +    inline bool shouldIncludeJSObjectPropertyNames() const

The inline keyword here is not needed and should be omitted.

> Source/JavaScriptCore/runtime/EnumerationMode.h:59
> +    inline bool shouldIncludeSymbols() const

The inline keyword here is not needed and should be omitted.

> Source/JavaScriptCore/runtime/EnumerationMode.h:64
> +    static inline EnumerationMode createThatSkipsJSObject(EnumerationMode mode)

The inline keyword here is not needed and should be omitted.

> Source/JavaScriptCore/runtime/EnumerationMode.h:70
> +    inline explicit EnumerationMode(unsigned flags)

The inline keyword here is not needed and should be omitted.

> Source/JavaScriptCore/runtime/Identifier.h:52
> +    Identifier(ExecState* exec, const String& s)
> +        : Identifier(exec, s.impl())
> +    {
> +    }

Once a function becomes multi-line like this, we should move the implementation out of the class definition and just mark it inline explicitly instead. It should go into the IdentifierInlines.h file. The reason is that we want the class definition to be readable and give an overview of the class's behavior, and that is no longer true if we have lots of function bodies. This class already has a bit of a problem with that, but adding more function bodies will make it even worse.

> Source/JavaScriptCore/runtime/Identifier.h:63
> +    Identifier(VM* vm, const String& s)
> +        : Identifier(vm, s.impl())
> +    {
> +    }

Shouldn’t this have an assertion: !s.isUnique()?

> Source/JavaScriptCore/runtime/Identifier.h:73
> +    static Identifier from(VM* vm, StringImpl* uid)

I don’t think that “from” is a suitably clear function name to distinguish the version that works with unique strings from the version that prohibits unique strings.

[Yusuke Suzuki]

Comment on attachment 247333 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247333&action=review

Thank you for your review!
For IncludeDontEnumPropertiesAndExcludeSymbols, what do you think of making ExcludeSymbols as default and drop its name from Enum. It makes IncludeDontEnumPropertiesAndExcludeSymbols to IncludeDontEnumProperties. And when including Symbols, IncludeDontEnumPropertiesAndIncludeSymbols is used.

>> Source/JavaScriptCore/ChangeLog:104
>> +        To just construct `Identifier` from string type, we can use `Identifier(VM*/ExecState*, const WTFString&)` instead.
> 
> Having two separate functions seems fine. But having one be the constructor and the other be named “from” does not make the difference between these functions clear at all. We need to name the functions to explain the difference in their behavior.

That's reasonable.
So I suggest that, dropping public identifier constructor for String and expose 2 functions,
* Identifier::fromUid
* Identifier::fromString

What do you think of?

>> Source/JavaScriptCore/JavaScriptCore.order:-940
>> -__ZN3JSC9Structure29getPropertyNamesFromStructureERNS_2VMERNS_17PropertyNameArrayENS_15EnumerationModeE
> 
> No need to edit order files. These are generated files that need not be updated by hand. If you rebase this patch, feel free to omit the changes to this file.

Thanks! I'll rebase it without order file change.

>> Source/JavaScriptCore/runtime/EnumerationMode.h:44
>> +    inline EnumerationMode(EnumerationModeRepresentationTag tag)
> 
> The inline keyword here is not needed and should be omitted.

Fixed.

>> Source/JavaScriptCore/runtime/EnumerationMode.h:49
>> +    inline bool shouldIncludeDontEnumProperties() const
> 
> The inline keyword here is not needed and should be omitted.

Fixed.

>> Source/JavaScriptCore/runtime/EnumerationMode.h:54
>> +    inline bool shouldIncludeJSObjectPropertyNames() const
> 
> The inline keyword here is not needed and should be omitted.

Fixed.

>> Source/JavaScriptCore/runtime/EnumerationMode.h:59
>> +    inline bool shouldIncludeSymbols() const
> 
> The inline keyword here is not needed and should be omitted.

Fixed.

>> Source/JavaScriptCore/runtime/EnumerationMode.h:64
>> +    static inline EnumerationMode createThatSkipsJSObject(EnumerationMode mode)
> 
> The inline keyword here is not needed and should be omitted.

Fixed.

>> Source/JavaScriptCore/runtime/EnumerationMode.h:70
>> +    inline explicit EnumerationMode(unsigned flags)
> 
> The inline keyword here is not needed and should be omitted.

Fixed.

>> Source/JavaScriptCore/runtime/Identifier.h:52
>> +    }
> 
> Once a function becomes multi-line like this, we should move the implementation out of the class definition and just mark it inline explicitly instead. It should go into the IdentifierInlines.h file. The reason is that we want the class definition to be readable and give an overview of the class's behavior, and that is no longer true if we have lots of function bodies. This class already has a bit of a problem with that, but adding more function bodies will make it even worse.

Make sense. Moving the definition to IdentifierInline.h and marking it with inline keeps readability.
I'll fix it.

>> Source/JavaScriptCore/runtime/Identifier.h:63
>> +    }
> 
> Shouldn’t this have an assertion: !s.isUnique()?

Should insert it! Thx.

>> Source/JavaScriptCore/runtime/Identifier.h:73
>> +    static Identifier from(VM* vm, StringImpl* uid)
> 
> I don’t think that “from” is a suitably clear function name to distinguish the version that works with unique strings from the version that prohibits unique strings.

So I suggest the 2 functions.
* fromUid(VM* vm, StringImpl* uid)
It accepts unique id and non-unique id StringImpl*, but it keeps uniquness of StringImpl* held by Identifier.

* fromString(VM* vm, const String&)
It just generates Identifier from String. It guaranteed that StringImpl* held by generated Identifier is not unique.

[Yusuke Suzuki]

Created attachment 247619 [details]
Patch

[WebKit Commit Bot]

Attachment 247619 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:29:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:30:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:31:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:32:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 4 in 139 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 247620 [details]
Patch

[Yusuke Suzuki]

Could any JavaScriptCore people take a look at this patch?

[Filip Pizlo]

Comment on attachment 247620 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247620&action=review

> Source/JavaScriptCore/runtime/Identifier.h:64
> +    template<unsigned charactersCount>
> +    static Identifier fromString(VM*, const char (&characters)[charactersCount]);
> +    template<unsigned charactersCount>
> +    static Identifier fromString(ExecState*, const char (&characters)[charactersCount]);
> +    static Identifier fromString(VM*, const LChar*, int length);
> +    static Identifier fromString(VM*, const UChar*, int length);
> +    static Identifier fromString(VM*, const String&);
> +    static Identifier fromString(ExecState*, AtomicStringImpl*);
> +    static Identifier fromString(ExecState*, const AtomicString&);
> +    static Identifier fromString(ExecState*, const String&);
> +    static Identifier fromString(ExecState*, const char*);
> +
> +    static Identifier fromUid(VM*, StringImpl* uid);
> +    static Identifier fromUid(ExecState*, StringImpl* uid);
> +    static Identifier fromUid(const PrivateName&);

You should have a beefy comment here to describe the difference, and how that difference is used.

[Yusuke Suzuki]

Created attachment 247836 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 247620 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247620&action=review

>> Source/JavaScriptCore/runtime/Identifier.h:64
>> +    static Identifier fromUid(const PrivateName&);
> 
> You should have a beefy comment here to describe the difference, and how that difference is used.

Thank you! I've added comments here and updated the revised patch :D

[Filip Pizlo]

Comment on attachment 247836 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247836&action=review

> Source/JavaScriptCore/ChangeLog:11
> +        This patch implements `Object.getOwnPropertySymbols`.
> +        One technical issue is that, since we use private symbols (such as `@Object`) in the
> +        privileged JS code in `builtin/`, they should not be exposed.
> +        To distinguish them from the usual symbols, `StringImpl*` represents whether it's private or not.

Is this really the only way?  If it's just builtin symbols, then you could imagine maintaining a set of them.  Like, literally HashSet<StringImpl*> JSGlobalObject::m_privates.

Did you consider that?

[Yusuke Suzuki]

Comment on attachment 247836 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247836&action=review

Thank you for your review!

>> Source/JavaScriptCore/ChangeLog:11
>> +        To distinguish them from the usual symbols, `StringImpl*` represents whether it's private or not.
> 
> Is this really the only way?  If it's just builtin symbols, then you could imagine maintaining a set of them.  Like, literally HashSet<StringImpl*> JSGlobalObject::m_privates.
> 
> Did you consider that?

Great pointing! Your suggestion looks very nice.
Marking private symbols with non-intrusive way has Pros/Cons.

Pros:
+ Dropping private information from StringImpl*, this is not essential information belongging to StringImpl* (btw, it doesn't reduce bit flag space since atomic & unique flags still occupies 2bits)

Cons:
+ We need to filter private symbols with hash set lookups in the phase of adding property names to PropertyNamesArray. It looks costly.

After considering about these pros/cons, I think your suggestion looks very clean way.
And performance cost would be limited because we need to filter private symbols only when the target StringImpl* is unique and we need to IncludeSymbols. So this performance impact only affects on Object.getOwnPropertySymbols case.
What do you think of?

[Yusuke Suzuki]

Created attachment 247933 [details]
Patch

[Filip Pizlo]

(In reply to comment #28)
> Comment on attachment 247836 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247836&action=review
> 
> Thank you for your review!
> 
> >> Source/JavaScriptCore/ChangeLog:11
> >> +        To distinguish them from the usual symbols, `StringImpl*` represents whether it's private or not.
> > 
> > Is this really the only way?  If it's just builtin symbols, then you could imagine maintaining a set of them.  Like, literally HashSet<StringImpl*> JSGlobalObject::m_privates.
> > 
> > Did you consider that?
> 
> Great pointing! Your suggestion looks very nice.
> Marking private symbols with non-intrusive way has Pros/Cons.
> 
> Pros:
> + Dropping private information from StringImpl*, this is not essential
> information belongging to StringImpl* (btw, it doesn't reduce bit flag space
> since atomic & unique flags still occupies 2bits)
> 
> Cons:
> + We need to filter private symbols with hash set lookups in the phase of
> adding property names to PropertyNamesArray. It looks costly.
> 
> After considering about these pros/cons, I think your suggestion looks very
> clean way.
> And performance cost would be limited because we need to filter private
> symbols only when the target StringImpl* is unique and we need to
> IncludeSymbols. So this performance impact only affects on
> Object.getOwnPropertySymbols case.
> What do you think of?

It's an interesting question.  I'd like to hear ggaren's thoughts on this.  I'm on the fence.  I like that your approach makes Object.getOwnPropertySymbols fast.  On the other hand, hash lookups can be made fast.  It's a tough call, so I'd like to see what others think.

[Yusuke Suzuki]

Comment on attachment 247836 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247836&action=review

>>>> Source/JavaScriptCore/ChangeLog:11
>>>> +        To distinguish them from the usual symbols, `StringImpl*` represents whether it's private or not.
>>> 
>>> Is this really the only way?  If it's just builtin symbols, then you could imagine maintaining a set of them.  Like, literally HashSet<StringImpl*> JSGlobalObject::m_privates.
>>> 
>>> Did you consider that?
>> 
>> Great pointing! Your suggestion looks very nice.
>> Marking private symbols with non-intrusive way has Pros/Cons.
>> 
>> Pros:
>> + Dropping private information from StringImpl*, this is not essential information belongging to StringImpl* (btw, it doesn't reduce bit flag space since atomic & unique flags still occupies 2bits)
>> 
>> Cons:
>> + We need to filter private symbols with hash set lookups in the phase of adding property names to PropertyNamesArray. It looks costly.
>> 
>> After considering about these pros/cons, I think your suggestion looks very clean way.
>> And performance cost would be limited because we need to filter private symbols only when the target StringImpl* is unique and we need to IncludeSymbols. So this performance impact only affects on Object.getOwnPropertySymbols case.
>> What do you think of?
> 
> It's an interesting question.  I'd like to hear ggaren's thoughts on this.  I'm on the fence.  I like that your approach makes Object.getOwnPropertySymbols fast.  On the other hand, hash lookups can be made fast.  It's a tough call, so I'd like to see what others think.

And one more possible solution is, folding private bit into StringImpl* itself.
Like following code,

class PropertyName {
public:
    PropertyName = delete;

    static PropertyName* createPublicName(StringImpl* impl)
    {
        return reinterpret_cast<PropertyName*>(impl);
    }

    static PropertyName* createPrivateName(StringImpl* impl)
    {
        uintptr_t pointer = reinterpret_cast<uintptr_t>(impl);
        return reinterpret_cast<PropertyName*>(pointer | 0x1);
    }

    bool isPrivateName() const
    {
        return reinterpret_cast<uintptr_t>(this) & 0x1;
    }

    void ref()
    {
        extractStringImpl()->ref();
    }

    bool hasOneRef() const
    {
        return extractStringImpl()->hasOneRef();
    }

    unsigned refCount() const
    {
        return extractStringImpl()->refCount();
    }

    void deref()
    {
        extractStringImpl()->deref();
    }

private:
    StringImpl* extractStringImpl() const
    {
        return reinterpret_cast<StringImpl*>(reinterpret_cast<uintptr_t>(this) & ~(0x1ULL));
    }
};

And storing this PropertyName* into PropertyTable.

Pros:
1. Very efficient
2. Private bit is separated from StringImpl*

Cons:
1. A bit complicated implementation
2. Possibly large changes are required in JSC

[Geoffrey Garen]

> > After considering about these pros/cons, I think your suggestion looks very
> > clean way.
> > And performance cost would be limited because we need to filter private
> > symbols only when the target StringImpl* is unique and we need to
> > IncludeSymbols. So this performance impact only affects on
> > Object.getOwnPropertySymbols case.
> > What do you think of?
> 
> It's an interesting question.  I'd like to hear ggaren's thoughts on this. 
> I'm on the fence.  I like that your approach makes
> Object.getOwnPropertySymbols fast.  On the other hand, hash lookups can be
> made fast.  It's a tough call, so I'd like to see what others think.

My intuition is that there is value in separating the concern of this ES feature out of a low-level class like StringImpl.

Also, I think the cost of a hash lookup in gathering symbol names is probably acceptable because:

(1) name iteration already requires us to hash-unique names in the prototype chain;

(2) the most important optimization in making name iteration fast is to cache the result in the structure.

That said, if a benchmark showed this hash lookup to be a problem, I would want to find a better solution, and I would consider putting bits in StringImpl as an option.

[Geoffrey Garen]

Comment on attachment 247933 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247933&action=review

> Source/JavaScriptCore/ChangeLog:12
> +        This patch implements `Object.getOwnPropertySymbols`.
> +        One technical issue is that, since we use private symbols (such as `@Object`) in the
> +        privileged JS code in `builtins/`, they should not be exposed.
> +        To distinguish them from the usual symbols, checking the target `StringImpl*` is a not private name
> +        before adding it into PropertyNameArray.

When is it possible to encounter a private symbol (like `@Object`) on an object?

I believe our implementation of builtins attempts to ensure that private symbols do not escape into any programmer-accessible namespace.

> Source/JavaScriptCore/ChangeLog:39
> +        + `Identifier::fromUid(VM*/ExecState*, StringImpl*)`.
> +            This function is used for 2) `uid`. So uniqueness of `StringImpl*` is kept.

Should we start saying "Symbol" now instead of "UID"?

> Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:524
> +                    if (name->isUnique()) {
> +                        if (!mode.shouldIncludeSymbols())
> +                            continue;

This would read more clearly to me if the function on StringImpl were "isSymbol".

Is there a difference between a symbol, a unique string, and a uid? Perhaps I am showing my own ignorance here.

If there is no difference -- and I think there is none -- let's pick just one name for this concept. After all, sometimes one has to review a patch after waking up at 6AM, and one's poor little brain can use all the help it can get. :)

> Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:526
> +                        if (exec->propertyNames().isPrivateName(name))
> +                            continue;

Perhaps this can just be an ASSERT? See my comment above about preventing private names from escaping.

> Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:545
> +                    if (name->isUnique()) {
> +                        if (!mode.shouldIncludeSymbols())
> +                            continue;
> +                        if (exec->propertyNames().isPrivateName(name))
> +                            continue;
> +                    }

Ditto.

[Geoffrey Garen]

> And one more possible solution is, folding private bit into StringImpl*
> itself.
> Like following code,
> 
> class PropertyName {

I think something like this might become desirable if the implementation of ES features related to property names adds even more features. For now, I think it might not be necessary to make such a large change.

For a bit of history on this subject, JavaScriptCore used to have its own string class, separate from StringImpl, and we worked hard to remove it, both for simplicity's sake, and to improve performance.

We could probably add back this PropertyName concept without harming performance -- but it would be nice not to have to.

[Yusuke Suzuki]

(In reply to comment #32)
> My intuition is that there is value in separating the concern of this ES
> feature out of a low-level class like StringImpl.
> 
> Also, I think the cost of a hash lookup in gathering symbol names is
> probably acceptable because:
> 
> (1) name iteration already requires us to hash-unique names in the prototype
> chain;
> 
> (2) the most important optimization in making name iteration fast is to
> cache the result in the structure.
> 
> That said, if a benchmark showed this hash lookup to be a problem, I would
> want to find a better solution, and I would consider putting bits in
> StringImpl as an option.

Reasonable. OK, so, I'll take the way separating StringImpl and private bits.
Thank you :)

[Yusuke Suzuki]

(In reply to comment #34)
> I think something like this might become desirable if the implementation of
> ES features related to property names adds even more features. For now, I
> think it might not be necessary to make such a large change.
> 
> For a bit of history on this subject, JavaScriptCore used to have its own
> string class, separate from StringImpl, and we worked hard to remove it,
> both for simplicity's sake, and to improve performance.
> 
> We could probably add back this PropertyName concept without harming
> performance -- but it would be nice not to have to.

Sounds reasonable.

[Yusuke Suzuki]

Comment on attachment 247933 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247933&action=review

Added comments.

>> Source/JavaScriptCore/ChangeLog:12
>> +        before adding it into PropertyNameArray.
> 
> When is it possible to encounter a private symbol (like `@Object`) on an object?
> 
> I believe our implementation of builtins attempts to ensure that private symbols do not escape into any programmer-accessible namespace.

I think, after landing Object.getOwnPropertySymbols, we can encounter with the following code without guarding isPrivateName check.

var global = ...;  // like (new Function("return this"))()
var symbols = Object.getOwnPropertySymbols(global);

Correct?

>> Source/JavaScriptCore/ChangeLog:39
>> +            This function is used for 2) `uid`. So uniqueness of `StringImpl*` is kept.
> 
> Should we start saying "Symbol" now instead of "UID"?

Currently this Identifier::fromUid also accepts StringImpl* uid that represents normal string (which is used for the usual string (non-symbol) property name).
This is because in the current JSC implementation StringImpl* is held as <Symbol|StringProperty> type. So constructing Identifier for String/Symbol property from StringImpl* like Identifier::fromUid is useful.

>> Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:524
>> +                            continue;
> 
> This would read more clearly to me if the function on StringImpl were "isSymbol".
> 
> Is there a difference between a symbol, a unique string, and a uid? Perhaps I am showing my own ignorance here.
> 
> If there is no difference -- and I think there is none -- let's pick just one name for this concept. After all, sometimes one has to review a patch after waking up at 6AM, and one's poor little brain can use all the help it can get. :)

Nice catch.
UID is different from Symbol beacuse it contains String property (It's <String|Symbol> id). But, isUnique is the same to isSymbol. So replacing isUnique with isSymbol makes the code clear.

>> Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:526
>> +                            continue;
> 
> Perhaps this can just be an ASSERT? See my comment above about preventing private names from escaping.

Right. Here, since the staticValues' key is generated by String::fromUTF8(const char*), the string is not atomic & unique. So here, there's no symbol.
I'll insert `ASSER(!name->isSymbol());` here :)

[Yusuke Suzuki]

Created attachment 248266 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 247933 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247933&action=review

>>> Source/JavaScriptCore/ChangeLog:39
>>> +            This function is used for 2) `uid`. So uniqueness of `StringImpl*` is kept.
>> 
>> Should we start saying "Symbol" now instead of "UID"?
> 
> Currently this Identifier::fromUid also accepts StringImpl* uid that represents normal string (which is used for the usual string (non-symbol) property name).
> This is because in the current JSC implementation StringImpl* is held as <Symbol|StringProperty> type. So constructing Identifier for String/Symbol property from StringImpl* like Identifier::fromUid is useful.

Calling StringImpl* as uid is derived from the fact that this type is actually stored in PropertyTable.
So, maybe, UID's *unique* comes from the meaning: "the uniqueness of Property name ID in PropertyTable".
Is it preferable to rename it to the other name? Personally, now I renamed `isUnique` to `isSymbol`, we can still use this "unique" meaning as "the uniqueness of Property name ID".

[Geoffrey Garen]

> I think, after landing Object.getOwnPropertySymbols, we can encounter with
> the following code without guarding isPrivateName check.
> 
> var global = ...;  // like (new Function("return this"))()
> var symbols = Object.getOwnPropertySymbols(global);
> 
> Correct?

I see.

I thought private names were simply implemented as a feature in the parser, which produced a constant node in the AST. But I was wrong: Private names are properties of the global object.

[Geoffrey Garen]

> Calling StringImpl* as uid is derived from the fact that this type is
> actually stored in PropertyTable.
> So, maybe, UID's *unique* comes from the meaning: "the uniqueness of
> Property name ID in PropertyTable".

Makes sense.

[Yusuke Suzuki]

Comment on attachment 248266 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248266&action=review

OK, ready for review :)

> Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:522
> +                    ASSERT(!name->isSymbol());

Here, `staticValues` key is created by String::fromUTF8 and copied by isolatedCopy in JSClassRef.cpp. So it should not be symbol.

> Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:535
> +                    ASSERT(!name->isSymbol());

ditto

[Timothy Hatcher]

Can we get this reviewed? It is blocking Web Inspector Symbol support in bug 141279.

[Filip Pizlo]

Comment on attachment 248266 [details]
Patch

Let's go with the HashSet of private symbols approach.  It's less intrusive.

[Yusuke Suzuki]

Created attachment 248642 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 248642 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248642&action=review

OK! I've updated the patch that takes non-intrusive way.

> Source/JavaScriptCore/builtins/BuiltinNames.h:95
> +    return m_privateToPublicMap.find(uid) != m_privateToPublicMap.end();

`BuiltinNames` already has `m_privateToPublicMap` to provide the function `BuiltinNames::getPrivateName(const Identifier& ident)`.
So all private symbols are already registered in this map :)
In this patch, we just leverage this existing map to query whether uid is private.

> Source/JavaScriptCore/runtime/JSLexicalEnvironment.cpp:124
> +                if (exec->propertyNames().isPrivateName(it->key.get()))

Use `isPrivateName` like this.
Private-ness is now held with non-intrusive way. HashMap in BuiltinNames (for getPrivateName function) manages all private symbols.

[Yusuke Suzuki]

I'll rebaseline the patch with WebCore buildfix, but it's trivial.

[Yusuke Suzuki]

Created attachment 248648 [details]
Patch

[Yusuke Suzuki]

Simply rebasing...

[Filip Pizlo]

This new approach involves doing a hash lookup to check if a name is private on most uses of property enumeration.  When we suggested this less-intrusive approach, we made that suggestion under the assumption that:

1) It would lead to a smaller patch.

2) That hash lookup cost would only be incurred when using the new Object.getOwnPropertySymbols API.

It appears that your implementation is a very large patch and we pay the hash lookup in a lot of places.

Why can't you change it so that the only place that consults the hashmap is Object.getOwnPropertySymbols?  That is, get rid of the new enumeration modes.  Don't change any of that code.  Don't change Structure, JSSymbolTableObject, etc.  Just put the filtering in Object.getOwnPropertySymbols.

Would that work?

[Yusuke Suzuki]

(In reply to comment #50)
> This new approach involves doing a hash lookup to check if a name is private
> on most uses of property enumeration.  When we suggested this less-intrusive
> approach, we made that suggestion under the assumption that:
> 
> 1) It would lead to a smaller patch.
> 
> 2) That hash lookup cost would only be incurred when using the new
> Object.getOwnPropertySymbols API.
> 
> It appears that your implementation is a very large patch and we pay the
> hash lookup in a lot of places.
> 
> Why can't you change it so that the only place that consults the hashmap is
> Object.getOwnPropertySymbols?  That is, get rid of the new enumeration
> modes.  Don't change any of that code.  Don't change Structure,
> JSSymbolTableObject, etc.  Just put the filtering in
> Object.getOwnPropertySymbols.
> 
> Would that work?

OK :) As the result of discussion on IRC, I'll change this to

1. Exclude Symbols functionality is shared. Implemented in JSObject::getOwnPropertyNames JSC APIs
2. Filtering private symbols functionality is implemented in objectConstructorGetOwnPropertySymbols

[Yusuke Suzuki]

Created attachment 249205 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 249205 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=249205&action=review

Updated the patch.
Move hash lookups to ObjectConstructor.cpp.
After IRC discussion, I grep-ed tree and found several use of IncludeSymbols enumeration mode (Object.seal, Object.freeze, Object.defineProperties (Object.create), Object.isFrozen, Object.isSealed, Object.getOwnPropertySymbols)
However, all these use are placed in ObjectConstructor.cpp.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:233
> +        if (impl->isSymbol() && !exec->propertyNames().isPrivateName(impl))

Using hash lookup.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:380
> +        if (propertyName.isSymbol() && exec->propertyNames().isPrivateName(propertyName))

Using hash lookup for Object.defineProperties.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:430
> +        if (propertyName.isSymbol() && exec->propertyNames().isPrivateName(propertyName))

Using hash lookup for Object.seal.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:470
> +        if (propertyName.isSymbol() && exec->propertyNames().isPrivateName(propertyName))

Using hash lookup for Object.freeze.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:521
> +        if (propertyName.isSymbol() && exec->propertyNames().isPrivateName(propertyName))

Using hash lookup for Object.isSealed.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:554
> +        if (propertyName.isSymbol() && exec->propertyNames().isPrivateName(propertyName))

Using hash lookup for Object.isFrozen.

[Yusuke Suzuki]

Could you take a look?

[Yusuke Suzuki]

Comment on attachment 249205 [details]
Patch

Let's split it into several patches.

[Yusuke Suzuki]

At first, I've split it into Identifier & StringImpl* changes.
https://bugs.webkit.org/show_bug.cgi?id=143146

[Yusuke Suzuki]

Next https://bugs.webkit.org/show_bug.cgi?id=143276

[Yusuke Suzuki]

Created attachment 250032 [details]
Patch

[Yusuke Suzuki]

Now, splitted 2 patches are landed.
So Object.getOwnPropertySymbols patch becomes fairly small :D

[Geoffrey Garen]

Comment on attachment 250032 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=250032&action=review

r=me with some fixes outlined below.

> Source/JavaScriptCore/ChangeLog:11
> +        To distinguish them from the usual symbols, checking the target `StringImpl*` is a not private name

checking => check

> Source/JavaScriptCore/builtins/BuiltinNames.h:97
> +    return m_privateToPublicMap.find(uid) != m_privateToPublicMap.end();

You can use contains() here instead.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:383
> +        if (propertyName.isSymbol() && exec->propertyNames().isPrivateName(propertyName))

Since isPrivateName checks isSymbol, we shouldn't check at the call site. It's an optimization, which is an implementation detail.

This pattern repeats in a few places.

[Yusuke Suzuki]

Comment on attachment 250032 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=250032&action=review

Thank you for your review!

>> Source/JavaScriptCore/ChangeLog:11
>> +        To distinguish them from the usual symbols, checking the target `StringImpl*` is a not private name
> 
> checking => check

Thanks!

>> Source/JavaScriptCore/builtins/BuiltinNames.h:97
>> +    return m_privateToPublicMap.find(uid) != m_privateToPublicMap.end();
> 
> You can use contains() here instead.

Looks very nice :D

>> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:383
>> +        if (propertyName.isSymbol() && exec->propertyNames().isPrivateName(propertyName))
> 
> Since isPrivateName checks isSymbol, we shouldn't check at the call site. It's an optimization, which is an implementation detail.
> 
> This pattern repeats in a few places.

OK. I'll fix these guards.

[Yusuke Suzuki]

Committed r182343: <http://trac.webkit.org/changeset/182343>