[webkit-reviews] review denied: [Bug 194706] Cache CompactVariableMap::Handle instead of VariableEnvironment for UnlinkedFunctionExecutable : [Attachment 362118] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied Tadeu Zagallo <tzagallo at apple.com>'s
request for review:
Bug 194706: Cache CompactVariableMap::Handle instead of VariableEnvironment for
UnlinkedFunctionExecutable
https://bugs.webkit.org/show_bug.cgi?id=194706

Attachment 362118: Patch

https://bugs.webkit.org/attachment.cgi?id=362118&action=review




--- Comment #2 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 362118
  --> https://bugs.webkit.org/attachment.cgi?id=362118
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362118&action=review

I think this is slightly off but I’m not 100% sure. Since Handle is responsible
for bumping/decrementing the ref count, I’m not sure we properly model that,
since we might decode only N handles, but have the ref count as M. I wonder if
it’d make more sense to iteratively build the ref count as we decode handles?

> Source/JavaScriptCore/runtime/CachedTypes.cpp:941
> +	   CompactVariableMap* map = new CompactVariableMap;

This looks like a leak to me. How is this supposed to be handled? Maybe this
should return Ref, and this should be adoptRef?

> Source/JavaScriptCore/runtime/CachedTypes.cpp:962
> +	   handle.m_map = m_map.decode(decoder);

I’m assuming the decoder already hash-conses stuff?

> Source/JavaScriptCore/runtime/CachedTypes.cpp:963
> +	   return handle;

It worries me that we somehow don’t end up updating the map’s ref count in
place. For example, when we encode a graph of code blocks, there may be
Handle’s that live outside that graph. If so, we’re now leaking those
VariableEnvironments since we’re encoding a global ref count. I think we should
be smart enough to rebuild it local to the map we’re re-creating.