[webkit-reviews] review granted: [Bug 224414] [JSC] Do not copy StringSwitchJumpTable : [Attachment 425699] Patch

[bugzilla-daemon at webkit.org]

Keith Miller <keith_miller at apple.com> has granted Yusuke Suzuki
<ysuzuki at apple.com>'s request for review:
Bug 224414: [JSC] Do not copy StringSwitchJumpTable
https://bugs.webkit.org/show_bug.cgi?id=224414

Attachment 425699: Patch

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




--- Comment #3 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 425699
  --> https://bugs.webkit.org/attachment.cgi?id=425699
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:8
> +	   Previously, we are copying UnlinkedStringJumpTable to CodeBlock's
StringJumpTable because we are embedding CodeLocation pointer

Nit: Since previously is past tense the verb should be were rather than are.
Also: "because we are embedding" => "because we embed"

> Source/JavaScriptCore/ChangeLog:9
> +	   inside CodeBlock's StringJumpTable. This is copying mostly identical
hashtable to each CodeBlock even in DFG and FTL. And this

Nit: "copying mostly" => "copying a mostly". You can also drop the "and in the
third sentence.

> Source/JavaScriptCore/ChangeLog:10
> +	   even prevents us from inlining op_switch_string in DFG and FTL.

Maybe explain why it prevents inlining.

> Source/JavaScriptCore/ChangeLog:15
> +	   from copying unnecessary hashtable, and even this paves the way to
inlining switch_string in DFG and FTL.

Nit: hashtables*