[Webkit-unassigned] [Bug 68329] DFG should support continuous optimization

[bugzilla-daemon at webkit.org]

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


Geoffrey Garen <ggaren at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #108106|review?                     |review-
               Flag|                            |




--- Comment #19 from Geoffrey Garen <ggaren at apple.com>  2011-09-20 22:47:25 PST ---
(From update of attachment 108106)
View in context: https://bugs.webkit.org/attachment.cgi?id=108106&action=review

Some good stuff here, but the ConservativeRoots thing needs reworking, and some refactoring would improve clarity.

> Source/JavaScriptCore/heap/ConservativeRoots.cpp:72
> +    m_codeBlocks->mark(p);

Doing a full pointer-hashed lookup on every pointer defeats a lot of prior optimization work on conservative root gathering, which was a win at the time. It also kind of confuses the role of the ConservativeRoots class, since the ConservativeRoots class is used to gather roots form anywhere -- currently, the C stack or the JS stack -- but you're only interested in CodeBlock entries in the JS stack (and then, only in call frames).

You should rework this.

One option is just to wait and blow away all jettisoned CodeBlocks unconditionally when exiting the VM. (In some cases, this is better, since there's no guarantee that a given VM invocation that fails lots of speculations will ever run the GC.)

Another option is to do a specific scan for CodeBlocks in the JS stack if and only if the set of jettisoned CodeBlocks reaches a threshold. (This scan can become much more efficient in the future, once JC's top-of-stack work is done, and we have exact backtrace information.)

Or you can combine the two options.

> Source/JavaScriptCore/heap/Heap.cpp:397
> +void Heap::jettisonCodeBlock(PassOwnPtr<CodeBlock> codeBlock)

Since this doesn't actually do the jettisoning, I'd call it "addJettisonedCodeBlock" instead.

> Source/JavaScriptCore/heap/Heap.h:197
> +        JettisonedCodeBlocks m_codeBlocks;

"m_jettisonedCodeBlocks", please. Otherwise, m_codeBlocks reads like it's the set of all CodeBlocks.

> Source/JavaScriptCore/heap/JettisonedCodeBlocks.cpp:42
> +    HashMap<CodeBlock*, bool>::iterator begin = m_map.begin();
> +    HashMap<CodeBlock*, bool>::iterator end = m_map.end();
> +    for (HashMap<CodeBlock*, bool>::iterator iter = begin; iter != end; ++iter)
> +        delete iter->first;

Would deleteAllKeys work here?

> Source/JavaScriptCore/heap/JettisonedCodeBlocks.h:72
> +    // It would be great to use an OwnPtr<CodeBlock> here but that would
> +    // almost certainly not work.

Yeah, HashMap doesn't support OwnPtr.

> Source/JavaScriptCore/wtf/BitVector.h:57
> +        : m_bitsOrPointer(static_cast<uintptr_t>(1) << maxInlineBits())

Since tagging bits with the inline tag comes up a lot, I'd suggest a helper function: uintptr_t makeInlineBits(uintptr_t bits). So, this would turn into a call to makeInlineBits(0). The helper function can also ASSERT that bits does not already have the tag bit set.

> Source/JavaScriptCore/wtf/BitVector.h:90
> +    size_t bounds() const

Since this is modified by resize() and ensureSize(), "size()" is probably a better name for it.

> Source/JavaScriptCore/wtf/BitVector.h:112
> +            m_bitsOrPointer = (static_cast<uintptr_t>(1) << maxInlineBits()) | *myOutOfLineBits->bits();

This would become "makeInlineBits(*myOutOfLineBits->bits())", which seems cleaner.

> Source/JavaScriptCore/wtf/BitVector.h:120
> +    void clear()

I think this would read a little better as "clearAll", matching Bitmap::clearAll. Our other collection types treat clear as setSize(0), so seeing clear() right after setSize(n) in client code is pretty weird.

> Source/JavaScriptCore/wtf/BitVector.h:125
> +            bzero(outOfLineBits()->bits(), bounds() >> 3);

>> 3 would be a little less magical as a helper function: size_t byteCount(size_t bitCount); Seems to come up a lot.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.