Bug 108029 – JSC: SourceProviderCache is memory hungry.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Andreas Kling]

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.

[Andreas Kling]

Created attachment 184911 [details]
Proposed patch

[WebKit Review Bot]

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.

[Build Bot]

Comment on attachment 184911 [details]
Proposed patch

Attachment 184911 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16138573

[Andreas Kling]

Created attachment 184912 [details]
Proposed patch v2

Let's see if we can #pragma our way out of MSVC's whining..

[Sam Weinig]

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.

[Radar WebKit Bug Importer]

<rdar://problem/13094806>

[Andreas Kling]

Committed r140945: <http://trac.webkit.org/changeset/140945>

[Geoffrey Garen]

Can we just remove the SourceProvider cache? Now that we cache bytecode, a parsing cache is pure overhead.

[Andreas Kling]

(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.

[Darin Adler]

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].

[Darin Adler]

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?

[Andreas Kling]

(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.