Bug 137315 - Object allocation sinking should have a sound story for picking materialization points
Summary: Object allocation sinking should have a sound story for picking materializati...
Status: RESOLVED FIXED
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: 137310
Blocks: 137126 137168
  Show dependency treegraph
 
Reported: 2014-10-01 14:46 PDT by Filip Pizlo
Modified: 2014-10-02 12:38 PDT (History)
10 users (show)

See Also:


Attachments
work in progress (23.14 KB, patch)
2014-10-02 12:05 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (24.01 KB, patch)
2014-10-02 12:14 PDT, Filip Pizlo
oliver: review+
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-10-01 14:46:47 PDT
The current code has roughly the right intuition for picking materialization sites: you need to forward flow the "I was materialized" property.  It's true that this is an important input.  Where the logic goes wrong is that it then tries to do materializations at the *escaping sites* where an object goes from "not materialized" to "materialized".  In the case of loops (and probably some crazy irreducible control flow) you may have a point where an object becomes materialized is not an escaping site.  The code should handle that case.
Comment 1 Filip Pizlo 2014-10-02 12:05:27 PDT
Created attachment 239126 [details]
work in progress
Comment 2 Filip Pizlo 2014-10-02 12:14:01 PDT
Created attachment 239127 [details]
the patch
Comment 3 WebKit Commit Bot 2014-10-02 12:15:56 PDT
Attachment 239127 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGGraph.h:854:  The parameter name "block" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Filip Pizlo 2014-10-02 12:16:30 PDT
(In reply to comment #3)
> Attachment 239127 [details] did not pass style-queue:
> 
> 
> ERROR: Source/JavaScriptCore/dfg/DFGGraph.h:854:  The parameter name "block" adds no information, so it should be removed.  [readability/parameter_name] [5]
> Total errors found: 1 in 11 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Fixed.
Comment 5 Oliver Hunt 2014-10-02 12:18:39 PDT
Comment on attachment 239127 [details]
the patch

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

> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:761
> +        Node* result;

For my sanity initialize result = nullptr;
Comment 6 Filip Pizlo 2014-10-02 12:23:59 PDT
(In reply to comment #5)
> (From update of attachment 239127 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239127&action=review
> 
> > Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:761
> > +        Node* result;
> 
> For my sanity initialize result = nullptr;

OK!
Comment 7 Filip Pizlo 2014-10-02 12:38:25 PDT
Landed in http://trac.webkit.org/changeset/174224