[webkit-reviews] review denied: [Bug 68329] DFG should support continuous optimization : [Attachment 108106] the patch - fix license

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Filip Pizlo <fpizlo at apple.com>'s
request for review:
Bug 68329: DFG should support continuous optimization
https://bugs.webkit.org/show_bug.cgi?id=68329

Attachment 108106: the patch - fix license
https://bugs.webkit.org/attachment.cgi?id=108106&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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.