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

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted 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 362700: Patch

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




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

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

r=me

> Source/JavaScriptCore/ChangeLog:12
> +	   that we cache the handle instead of the environment.

You should add a sentence why

> Source/JavaScriptCore/runtime/CachedTypes.cpp:380
> +	   if (WTF::Optional<ptrdiff_t> offset =
encoder.cachedOffsetForPtr(src)) {

Not this patch, but I don’t think the WTF is needed for Optional

> Source/JavaScriptCore/runtime/CachedTypes.cpp:940
> +	   CompactVariableEnvironment* environment =
m_environment.decode(decoder, isNewAllocation);

This can be a future improvement, but it would be cool if we could skip the
hash table lookup for all !isNewAllocation. This could work if we associated a
Handle instead of an environment for this pointer. This is sound because a
pointer to the same CompactVariableEnvironment* will hash the same. And this is
a speed up because hashing CompactVariableEnvironment* isn’t cheap