Bug 128072 – [ftlopt] GC should keep structures alive if they are inlined into optimized code (DFG or FTL) and their globalObject is alive

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: JavaScriptCore

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 222934 [details]
the patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:354:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:414:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:419:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:424:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 4 in 3 files


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

[Filip Pizlo]

Created attachment 222935 [details]
the patch

[Geoffrey Garen]

Comment on attachment 222935 [details]
the patch

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

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2059
> +// Returns false if we want this reconsidered.
> +static void forceStructureLiveness(

I think this comment got out of date while you were writing the patch.

[Filip Pizlo]

(In reply to comment #4)
> (From update of attachment 222935 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222935&action=review
> 
> > Source/JavaScriptCore/bytecode/CodeBlock.cpp:2059
> > +// Returns false if we want this reconsidered.
> > +static void forceStructureLiveness(
> 
> I think this comment got out of date while you were writing the patch.

Yup, removed.

[Filip Pizlo]

Further testing shows that this is basically blocked on https://bugs.webkit.org/show_bug.cgi?id=128073, and perhaps something even deeper than that: we need to be able to add the ability to:

- Track larger StructureSets for prorotype/chain access inlining.
- Convert GetByIds to inlined prototype/chain accesses.

Also, I think that this experience is suggesting that our baseline PIC's are going polymorphic too eagerly.  Perhaps a good heuristic would be to let them ping-pong N times before going polymorphic.

[Filip Pizlo]

Created attachment 223213 [details]
another version

I no longer think that this is a particularly good idea.

[Csaba Osztrogonác]

Comment on attachment 222935 [details]
the patch

Cleared Oliver Hunt's review+ from obsolete attachment 222935 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Filip Pizlo]

Created attachment 233923 [details]
work in progress

This uses a slightly different approach.  It's almost done.

[Filip Pizlo]

Created attachment 233951 [details]
the patch

[Geoffrey Garen]

Comment on attachment 233951 [details]
the patch

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

r=me, with some fixate

> Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp:85
> +        if (false && jit.codeBlock()->instructionCount() == 681)
> +            jit.breakpoint();

Please remove.

> Source/JavaScriptCore/runtime/Executable.h:457
> +    bool m_didTryToEnterInLoop;

It looks like this data member is not initialized in the constructor.

[Filip Pizlo]

Nooope.  Testing shows that actually, even with all of the work I did, the GC clearing structures is still profitable.

That said, we're now in a situation where it is less profitable on gbemu (i.e. it's not absolutely necessary for performance) and less evil (it's not necessary to assume that if the GC clears IC's then the IC's need to be forever polymorphic).

[Csaba Osztrogonác]

Comment on attachment 233951 [details]
the patch

Cleared Geoffrey Garen's review+ from obsolete attachment 233951 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Filip Pizlo]

Oh we did something much stronger than this.

*** This bug has been marked as a duplicate of bug 157324 ***