| 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 <saam> | ||||||
| Component: | JavaScriptCore | Assignee: | Saam Barati <saam> | ||||||
| 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
Saam Barati
2016-10-15 13:32:34 PDT
Created attachment 291843 [details]
patch
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. Created attachment 291847 [details]
patch for landing
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 on attachment 291847 [details] patch for landing Clearing flags on attachment: 291847 Committed r207425: <http://trac.webkit.org/changeset/207425> All reviewed patches have been landed. Closing bug. |