.
Created attachment 223898 [details] Rage hacked, builds, fixed ASSERTions & infinite loops, but almost all tests still failing!
Created attachment 226987 [details] New patch
JS Perf looks fine. All benchmarks: <arithmetic> 149.9601+-1.3387 148.4964+-0.8381 might be 1.0099x faster <geometric> 19.2674+-0.0367 ^ 19.1212+-0.0932 ^ definitely 1.0076x faster <harmonic> 4.2925+-0.0201 4.2662+-0.0586 might be 1.0062x faster
Oh, patch needs Changelogs & some of those inlines could do with their own header.
Attachment 226987 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/Identifier.h:113: The parameter name "r" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/Identifier.h:114: The parameter name "r" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 226997 [details] New fix Still needs changelog, etc.
Freakin' awesome!
(In reply to comment #7) > Freakin' awesome! +1
Comment on attachment 226997 [details] New fix View in context: https://bugs.webkit.org/attachment.cgi?id=226997&action=review > Source/JavaScriptCore/runtime/SmallStrings.cpp:62 > m_reps[i] = StringImpl::createSubstringSharingImpl(baseString, i, 1); > + m_reps[i] = AtomicString::add(m_reps[i].get()); Why use m_reps[i] for the intermediate value? Why not do this all on one line. > Source/JavaScriptCore/runtime/VM.cpp:198 > + , propertyNames(0) nullptr? > Source/WTF/wtf/text/AtomicStringTable.cpp:56 > + for (auto iter : m_table) Should name this "string", not "iter". I prefer "auto*" or "auto&" here to make it clear we don’t have any bad copying going on. > Source/WTF/wtf/text/AtomicStringTable.cpp:60 > void AtomicStringTable::destroy(AtomicStringTable* table) Do we even need this function any more since we have a public destructor now?
Comment on attachment 226997 [details] New fix View in context: https://bugs.webkit.org/attachment.cgi?id=226997&action=review Any of these functions that should take or return references rather than pointers? > Source/JavaScriptCore/runtime/VM.h:296 > + IdentifierTable* atomicStringTable() const { return identifierTable; } Consider returning a reference instead of a pointer? Why do we need this function? Just to rename the identifierTable data member? > Source/WTF/wtf/WTFThreadData.h:55 > +typedef AtomicStringTable IdentifierTable; Long term would we just get rid of this typedef?
(In reply to comment #9) > Why use m_reps[i] for the intermediate value? Why not do this all on one line. > > nullptr? > > I prefer "auto*" or "auto&" here to make it clear we don’t have any bad copying going on. done, done, done. > > Source/WTF/wtf/text/AtomicStringTable.cpp:60 > > void AtomicStringTable::destroy(AtomicStringTable* table) > > Do we even need this function any more since we have a public destructor now? Short answer, yes. Long answer, no. We still need this because it's used as a part of the mechanism for the main thread / web thread to decide who gets to delete their shared string table. But it's probably an over engineered solution – we probably just an if statement in WTFThreadData's destructor. Let me revise in a follow-on patch. (In reply to comment #10) > > > Source/JavaScriptCore/runtime/VM.h:296 > > + IdentifierTable* atomicStringTable() const { return identifierTable; } > > Consider returning a reference instead of a pointer? done, > Why do we need this function? Just to rename the identifierTable data member? We use this method so you can call AtomicString::add(VM&, StringImpl*), or AtomicString::add(ExecState&, StringImpl*), template on the first argument type to avoid a layering violation. Hopefully we may be ablate make this go away in a followup patch, if WTFThreadData access is now fast enough we make be able to always look up the string table from the thread data, rather than reading it off the VM in JSC. > > Source/WTF/wtf/WTFThreadData.h:55 > > +typedef AtomicStringTable IdentifierTable; > > Long term would we just get rid of this typedef? Yep, definitely. I'll do a followup purge of WTF for use of the work 'Identifier' (e.g. we should probably also remove isIdentifier, all uses -> isAtomic).
Created attachment 227221 [details] review fixes, windows build fix, separate out new file. Still missing ChangeLogs, and 2 JSC tests are failing.
Created attachment 227265 [details] Tests pass, think the JSC/WTF changes are probably complete. Should probably take another look at that WebCore change, and the Windows build is probably still broke (related). I've added JSC/WTF Changelogs, but still needs some description.
Created attachment 227312 [details] Patch, with speculative Windows build fix
Comment on attachment 227312 [details] Patch, with speculative Windows build fix View in context: https://bugs.webkit.org/attachment.cgi?id=227312&action=review r=me if it blends > Source/JavaScriptCore/ChangeLog:9 > + WTF::StringImpl currently supports two uniquing mechanism - AtomicString and > + Identifer - that is one too many. So, you're saying we should unique our uniquing?
Transmitting file data ............................ Committed revision 165982.
Build fix in revision 165990.
w00t! https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-mavericks%22%2C%22Bindings%2Fcreate-element%3ARuns%22%5D%2C%5B%22mac-mavericks%22%2C%22Bindings%2Fget-attribute%3ARuns%22%5D%2C%5B%22mac-mavericks%22%2C%22Bindings%2Fget-element-by-id%3ARuns%22%5D%2C%5B%22mac-mavericks%22%2C%22Bindings%2Fget-elements-by-tag-name%3ARuns%22%5D%2C%5B%22mac-mavericks%22%2C%22Bindings%2Fid-setter%3ARuns%22%5D%2C%5B%22mac-mavericks%22%2C%22Bindings%2Fset-attribute%3ARuns%22%5D%5D&days=30
*** Bug 38015 has been marked as a duplicate of this bug. ***