SourceProviderCacheItem currently has two vectors for captured variable names. Since these vectors are never mutated after creation, we could store them as fixed-sized arrays instead and bake the whole thing into a single allocation. I have a patch for this that yields a 13.3 MB (~2%) progression on Membuster3.
Created attachment 184911 [details] Proposed patch
Attachment 184911 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/parser/Parser.cpp', u'Source/JavaScriptCore/parser/Parser.h', u'Source/JavaScriptCore/parser/SourceProviderCacheItem.h']" exit_code: 1 Source/JavaScriptCore/parser/SourceProviderCacheItem.h:31: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 184911 [details] Proposed patch Attachment 184911 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16138573
Created attachment 184912 [details] Proposed patch v2 Let's see if we can #pragma our way out of MSVC's whining..
Comment on attachment 184912 [details] Proposed patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=184912&action=review > Source/JavaScriptCore/parser/SourceProviderCacheItem.h:92 > + ~SourceProviderCacheItem(); This is more common near the top of the class. > Source/JavaScriptCore/parser/SourceProviderCacheItem.h:110 > + void* slot = WTF::fastMalloc(objectSize); I don't think the WTF:: here is needed.
<rdar://problem/13094806>
Committed r140945: <http://trac.webkit.org/changeset/140945>
Can we just remove the SourceProvider cache? Now that we cache bytecode, a parsing cache is pure overhead.
(In reply to comment #8) > Can we just remove the SourceProvider cache? Now that we cache bytecode, a parsing cache is pure overhead. If we can, that would be another 10.12 MB saved on the benchmark.
Comment on attachment 184912 [details] Proposed patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=184912&action=review > Source/JavaScriptCore/parser/SourceProviderCacheItem.h:97 > + StringImpl* m_variables[0]; Could this have been done with RefPtr<StringImpl> instead? Here’s why I ask: > Source/JavaScriptCore/parser/SourceProviderCacheItem.h:103 > + m_variables[i]->deref(); This line of code could have been m_variables[i]::~RefPtr<StringImpl>(). > Source/JavaScriptCore/parser/SourceProviderCacheItem.h:127 > + m_variables[j] = parameters.usedVariables[i].get(); > + m_variables[j]->ref(); This line of code could have been m_variables[j] = parameters.usedVariables[i]. > Source/JavaScriptCore/parser/SourceProviderCacheItem.h:131 > + m_variables[j] = parameters.writtenVariables[i].get(); > + m_variables[j]->ref(); This line of code could have been m_variables[j] = parameters.usedVariables[i].
Comment on attachment 184912 [details] Proposed patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=184912&action=review > Source/JavaScriptCore/parser/Parser.h:340 > + copyCapturedVariablesToVector(m_writtenVariables, parameters.writtenVariables); > + copyCapturedVariablesToVector(m_usedVariables, parameters.usedVariables); The shrinkToFit call inside copyCapturedVariablesToVector is no longer a useful optimization, and I suggest we remove it, since the vectors are now temporaries. On the other hand, a reserveInitialCapacity call inside copyCapturedVariablesToVector could cut down on memory allocation. Or even an inline capacity in the vector type. Not sure if these would help performance, but perhaps. > Source/JavaScriptCore/parser/Parser.h:352 > + for (unsigned i = 0; i < info->usedVariablesCount; ++i) > + m_usedVariables.add(info->usedVariables()[i]); > + for (unsigned i = 0; i < info->writtenVariablesCount; ++i) > + m_writtenVariables.add(info->writtenVariables()[i]); Could this be made more efficient by using reserveCapacity or reserveInitialCapacity?
(In reply to comment #10) > (From update of attachment 184912 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=184912&action=review > > > Source/JavaScriptCore/parser/SourceProviderCacheItem.h:97 > > + StringImpl* m_variables[0]; > > Could this have been done with RefPtr<StringImpl> instead? Here’s why I ask: Yes. > > Source/JavaScriptCore/parser/SourceProviderCacheItem.h:103 > > + m_variables[i]->deref(); > > This line of code could have been m_variables[i]::~RefPtr<StringImpl>(). Sure, that would be nicer. > > Source/JavaScriptCore/parser/SourceProviderCacheItem.h:127 > > + m_variables[j] = parameters.usedVariables[i].get(); > > + m_variables[j]->ref(); > > This line of code could have been m_variables[j] = parameters.usedVariables[i]. We'd have to use placement new for these, since m_variables[j] is uninitialized at this point. (In reply to comment #11) > (From update of attachment 184912 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=184912&action=review > > > Source/JavaScriptCore/parser/Parser.h:340 > > + copyCapturedVariablesToVector(m_writtenVariables, parameters.writtenVariables); > > + copyCapturedVariablesToVector(m_usedVariables, parameters.usedVariables); > > The shrinkToFit call inside copyCapturedVariablesToVector is no longer a useful optimization, and I suggest we remove it, since the vectors are now temporaries. Indeed, which is why I removed it in this patch. :) > On the other hand, a reserveInitialCapacity call inside copyCapturedVariablesToVector could cut down on memory allocation. Or even an inline capacity in the vector type. > > Not sure if these would help performance, but perhaps. Giving it an inline capacity would be a straightforward way to remove at least two (transient) mallocs. > > Source/JavaScriptCore/parser/Parser.h:352 > > + for (unsigned i = 0; i < info->usedVariablesCount; ++i) > > + m_usedVariables.add(info->usedVariables()[i]); > > + for (unsigned i = 0; i < info->writtenVariablesCount; ++i) > > + m_writtenVariables.add(info->writtenVariables()[i]); > > Could this be made more efficient by using reserveCapacity or reserveInitialCapacity? HashSet doesn't have a reserveCapacity mechanism.