Bug 158032 – LLInt should be able to cache prototype loads for values in GetById

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

LLInt should be able to cache prototype loads for values in GetById

[Keith Miller]

Created attachment 279687 [details]
Patch

[Keith Miller]

Created attachment 279696 [details]
Patch

[Filip Pizlo]

Comment on attachment 279696 [details]
Patch

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

I think that this would be even better if it integrated with GetByIdStatus.  Would that be hard?

> Source/JavaScriptCore/bytecode/GetByIdStatus.cpp:81
> +    if (opcode == LLInt::getOpcode(op_get_array_length) || opcode == LLInt::getOpcode(op_try_get_by_id) || opcode == LLInt::getOpcode(op_get_by_id_proto_load))

It would be better if this didn't bail on op_get_by_id_proto_load.  Can you add a FIXME that links to a bug?

> Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.h:81
> +    bool isValidAndWatchable() const
> +    {
> +        if (!isValid())
> +            return false;
> +
> +        for (ObjectPropertyCondition condition : m_data->vector) {
> +            if (!condition.isWatchable())
> +                return false;
> +        }
> +        return true;
> +    }

You should move the body out of line.  Also, I'd call this isWatchable() because if you're not valid then you're not watchable.

[Geoffrey Garen]

Needs rebasing.

> Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.h:49
> +inline void clearLLIntGetByIdCache(Instruction* instruction)

This belongs in CodeBlock.h.

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:618
> +    // We know that this pointer will remain valid because it is the prototype of some structure, s,
> +    // watchpointed above. If any object with structure s were to change prototypes then the conditions
> +    // for this cache would fail and this value will never be used again.

It is hard to reason about having a dangling pointer and proving that you won't use it.

clearLLIntGetByIdCache() should clear this pointer.

Then, you can just point out that this pointer is guarded by a watchpoint, and the watchpoint will set this pointer to nullptr if necessary.

> Source/WTF/wtf/Bag.h:64
> +    Bag(Bag<T>&& other)
> +    {
> +        ASSERT(!m_head);
> +        std::swap(m_head, other.m_head);
> +    }
> +
> +    Bag& operator=(Bag<T>&& other)
> +    {
> +        std::swap(m_head, other.m_head);
> +        return *this;
> +    }

It's generally bad style to use swap in move construction or assignment. The reason is that order of destruction sometimes matters, and the swap approach produces strange destruction order. It's possible to convince yourself through analysis that your particular class does not care about order of destruction, but it's best not to have to do that kind of analysis. I would say instead:

m_head = other.m_head;
other.m_head = nullptr;

[Keith Miller]

Created attachment 279703 [details]
Patch

[Keith Miller]

Created attachment 279704 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 279704 [details]
Patch for landing

Rejecting attachment 279704 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 279704, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/JavaScriptCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/1376656

[Keith Miller]

Created attachment 279706 [details]
Patch for landing

[Keith Miller]

Created attachment 279708 [details]
Patch for landing

[Keith Miller]

Created attachment 279709 [details]
Patch for landing

[WebKit Commit Bot]

Attachment 279709 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/CMakeLists.txt:206:  Alphabetical sorting problem. "bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.cpp" should be before "bytecode/LazyOperandValueProfile.cpp".  [list/order] [5]
Total errors found: 1 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 279712 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 279712 [details]
Patch for landing

Clearing flags on attachment: 279712

Committed r201363: <http://trac.webkit.org/changeset/201363>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 158240

[Keith Miller]

This was rolled back in by http://trac.webkit.org/changeset/201617