[Webkit-unassigned] [Bug 154808] NewRegexp should not prevent inlining

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=154808

Geoffrey Garen <ggaren at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #286601|review?                     |review-
              Flags|                            |

--- Comment #6 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 286601
  --> https://bugs.webkit.org/attachment.cgi?id=286601
Patch

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

I think a better model might be for every CodeBlock always to copy its RegExp array out of its unlinked counterpart and into a unique vector. This will waste a little space, but we think it's rare, so it's probably acceptable. The nice thing about this change is that we won't need two different modes for accessing regexps.

If we do need two different modes to save space, I would call the first mode "unlinkedRegExp()" and the second mode "jitCodeRegExp()", and I would store the regexeps used only by optimizing compilers on the JITCode object.

> Source/JavaScriptCore/ChangeLog:9
> +        when it contains NewRegExp nodes. In current implementation, it is not

when the callee contains

In the current implementation, inlining

> Source/JavaScriptCore/ChangeLog:10
> +        possible because the RegExp references are being stored in their function's

are stored

> Source/JavaScriptCore/ChangeLog:12
> +        NewRegExp node is emmited with the RegExp's index in CodeBlock vector

emitted

in the CodeBlock vector

> Source/JavaScriptCore/ChangeLog:13
> +        as parameter to be retrived in futher phases of compilation

as a parameter

retrieved

> Source/JavaScriptCore/ChangeLog:16
> +        As solution, we are remmaping the RegExps from its callee inline canditate

As a solution, we remap each RegExp

candidate

> Source/JavaScriptCore/ChangeLog:17
> +        to its machine code block in inline phase into

in the inlining phase

> Source/JavaScriptCore/ChangeLog:24
> +        It is important notice that we created a new Vector because

to notice

> Source/JavaScriptCore/ChangeLog:27
> +        out of compilation thread.

from the compilation thread

> Source/JavaScriptCore/bytecode/CodeBlock.h:557
> +    unsigned addRegExpList(RegExp* regexp)

This function name sounds like you're adding a list of RegExps. I don't think it differs clearly enough from CodeBlock::regexp(). It is a little surprising that CodeBlock has two different RegExp stores, one that forwards to the unlinked code block and one that is local.

> Source/JavaScriptCore/bytecode/CodeBlock.h:900
> +        Vector<RegExp*> m_regExps;

Why is it OK to store raw pointers to RegExp without a write barrier operation? You've explained why we can't do write barrier operations on the compilation thread, but I don't think you've explained why it's OK just to skip them -- and I don't think it is OK.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:5401
> +        

Revert.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:5422
> +        

Revert.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:5428
> +    // We need to remap and copy RegExps from codeBlock->unlinkedCodeBlock() to
> +    // byteCodeParser->m_codeBlock because it allow us inline functions with NewRegExp.
> +    // nodes.

I don't fully understand this comment.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160822/20a52686/attachment-0001.html>