Bug 163490 - Build fix for HasOwnPropertyCache::Entry caused slowdown by introducing a constructor that doesn't use move semantics for the RefPtr<UniquedStringImpl> parameter
Summary: Build fix for HasOwnPropertyCache::Entry caused slowdown by introducing a con...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-10-15 13:32 PDT by Saam Barati
Modified: 2016-10-17 13:37 PDT (History)
14 users (show)

See Also:

Attachments
patch (1.92 KB, patch)
2016-10-17 11:33 PDT, Saam Barati 		darin: review+
Details | Formatted Diff | Diff
patch for landing (1.92 KB, patch)
2016-10-17 11:42 PDT, Saam Barati 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.