Bug 128072 - [ftlopt] GC should keep structures alive if they are inlined into optimized code (DFG or FTL) and their globalObject is alive
Summary: [ftlopt] GC should keep structures alive if they are inlined into optimized c...
Status: RESOLVED DUPLICATE of bug 157324
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on: 134427
Blocks: 128078 128079 132357
  Show dependency treegraph
 
Reported: 2014-02-02 10:08 PST by Filip Pizlo
Modified: 2016-05-19 14:07 PDT (History)
14 users (show)

See Also:


Attachments
the patch (23.54 KB, patch)
2014-02-02 12:52 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (23.68 KB, patch)
2014-02-02 12:57 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
another version (24.75 KB, patch)
2014-02-04 22:05 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
work in progress (20.13 KB, patch)
2014-06-26 12:20 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (22.86 KB, patch)
2014-06-26 18:04 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2014-02-02 10:08:41 PST
Patch forthcoming.
Comment 1 Filip Pizlo 2014-02-02 12:52:44 PST
Created attachment 222934 [details]
the patch
Comment 2 WebKit Commit Bot 2014-02-02 12:54:12 PST
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.
Comment 3 Filip Pizlo 2014-02-02 12:57:30 PST
Created attachment 222935 [details]
the patch
Comment 4 Geoffrey Garen 2014-02-02 14:51:13 PST
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.
Comment 5 Filip Pizlo 2014-02-02 21:36:15 PST
(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.
Comment 6 Filip Pizlo 2014-02-02 21:43:16 PST
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.
Comment 7 Filip Pizlo 2014-02-04 22:05:55 PST
Created attachment 223213 [details]
another version

I no longer think that this is a particularly good idea.
Comment 8 Csaba Osztrogonác 2014-02-13 04:08:36 PST
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.
Comment 9 Filip Pizlo 2014-06-26 12:20:21 PDT
Created attachment 233923 [details]
work in progress

This uses a slightly different approach.  It's almost done.
Comment 10 Filip Pizlo 2014-06-26 18:04:37 PDT
Created attachment 233951 [details]
the patch
Comment 11 Geoffrey Garen 2014-06-27 11:08:44 PDT
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.
Comment 12 Filip Pizlo 2014-06-27 20:33:37 PDT
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).
Comment 13 Csaba Osztrogonác 2014-07-16 02:43:55 PDT
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.
Comment 14 Filip Pizlo 2016-05-19 14:07:05 PDT
Oh we did something much stronger than this.

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