Bug 163490

Summary: Build fix for HasOwnPropertyCache::Entry caused slowdown by introducing a constructor that doesn't use move semantics for the RefPtr<UniquedStringImpl> parameter
Product: WebKit Reporter: Saam Barati <sbarati>
Component: JavaScriptCoreAssignee: Saam Barati <sbarati>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, darin, fpizlo, ggaren, gskachkov, jfbastien, keith_miller, mark.lam, msaboff, oliver, ossy, ticaiolima, ysuzuki
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
darin: review+
patch for landing none

Description Saam Barati 2016-10-15 13:32:34 PDT
...
Comment 1 Saam Barati 2016-10-17 11:33:23 PDT
Created attachment 291843 [details]
patch
Comment 2 Darin Adler 2016-10-17 11:38:18 PDT
Comment on attachment 291843 [details]
patch

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

> Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:48
> +        Entry(RefPtr<UniquedStringImpl>&& impl, StructureID structureID, bool result)
> +            : impl(WTFMove(impl))

Looks good.

> Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:124
> -            bitwise_cast<Entry*>(this)[index] = Entry{ RefPtr<UniquedStringImpl>(impl), id, result };
> +            bitwise_cast<Entry*>(this)[index] = Entry(RefPtr<UniquedStringImpl>(impl), id, result);

I suggest not making this change.

Using the function-call-style () syntax instead of the initialization-style {} syntax allows some kinds of implicit type conversions and it’s nice to not allow them. Generally, I think the { } syntax should be preferred in new code. In addition, Anders Carlsson convinced me that we should have a space after the type name, but I won’t try to convince you of that right now.
Comment 3 Saam Barati 2016-10-17 11:42:07 PDT
Created attachment 291847 [details]
patch for landing
Comment 4 Saam Barati 2016-10-17 11:44:07 PDT
Comment on attachment 291843 [details]
patch

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

>> Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:124
>> +            bitwise_cast<Entry*>(this)[index] = Entry(RefPtr<UniquedStringImpl>(impl), id, result);
> 
> I suggest not making this change.
> 
> Using the function-call-style () syntax instead of the initialization-style {} syntax allows some kinds of implicit type conversions and it’s nice to not allow them. Generally, I think the { } syntax should be preferred in new code. In addition, Anders Carlsson convinced me that we should have a space after the type name, but I won’t try to convince you of that right now.

Ok, I've changed it back, and with the space between the type name and the "{". I actually prefer the space, however, I've never been aware of any style rules for WebKit itself. If we haven't decided on an official style, we probably should, and put it in the style guide.

I like the property of not allowing type conversions and allowing the caller to control that instead of the callee.
Comment 5 WebKit Commit Bot 2016-10-17 13:36:57 PDT
Comment on attachment 291847 [details]
patch for landing

Clearing flags on attachment: 291847

Committed r207425: <http://trac.webkit.org/changeset/207425>
Comment 6 WebKit Commit Bot 2016-10-17 13:37:02 PDT
All reviewed patches have been landed.  Closing bug.