Bug 188707 - intersectionOfPastValuesAtHead must filter values after they've observed an invalidation point
Summary: intersectionOfPastValuesAtHead must filter values after they've observed an i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-08-17 14:21 PDT by Saam Barati
Modified: 2018-08-20 17:29 PDT (History)
13 users (show)

See Also:


Attachments
patch (5.05 KB, patch)
2018-08-17 14:57 PDT, Saam Barati
mark.lam: review+
Details | Formatted Diff | Diff
patch for landing (5.04 KB, patch)
2018-08-17 15:38 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch for landing (5.16 KB, patch)
2018-08-17 16:48 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch for landing (5.16 KB, patch)
2018-08-17 16:49 PDT, Saam Barati
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2018-08-17 14:21:28 PDT
...
Comment 1 Saam Barati 2018-08-17 14:22:32 PDT
<rdar://problem/43015442>
Comment 2 Saam Barati 2018-08-17 14:57:23 PDT
Created attachment 347392 [details]
patch
Comment 3 Mark Lam 2018-08-17 15:31:14 PDT
Comment on attachment 347392 [details]
patch

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

r=me

> Source/JavaScriptCore/ChangeLog:29
> +        We fix this by filtering values in intersectionOfPastValuesAtHead as
> +        with values as if an invalidation point has occured. This places the

The text "intersectionOfPastValuesAtHead as with values as if" is unclear.  Looks like there's some partial edits here.  Please fix.
Comment 4 Saam Barati 2018-08-17 15:38:24 PDT
Created attachment 347398 [details]
patch for landing
Comment 5 Geoffrey Garen 2018-08-17 16:33:12 PDT
Comment on attachment 347398 [details]
patch for landing

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

> Source/JavaScriptCore/dfg/DFGCFAPhase.cpp:147
> +                    // the incoming value as if an invalidation point has occurred.

as if it could live past an invalidation point

> Source/JavaScriptCore/dfg/DFGCFAPhase.cpp:149
> +                    // structure, and an InvalidationPoint no longer guarantees

It’s more like we’re violating the promise made by an invalidation point. The invalidating point itself still works.  We must conservatively act as if there’s an invalidation point because we do not pr Isley model them in osr entry metadata.
Comment 6 Saam Barati 2018-08-17 16:48:11 PDT
Created attachment 347412 [details]
patch for landing
Comment 7 Saam Barati 2018-08-17 16:49:48 PDT
Created attachment 347414 [details]
patch for landing
Comment 8 WebKit Commit Bot 2018-08-17 18:56:55 PDT
Comment on attachment 347414 [details]
patch for landing

Rejecting attachment 347414 [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-01', 'validate-changelog', '--check-oops', '--non-interactive', 347414, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in JSTests/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/8897685
Comment 9 Saam Barati 2018-08-17 19:05:29 PDT
landed in:
https://trac.webkit.org/changeset/235007/webkit
Comment 10 Filip Pizlo 2018-08-18 09:28:02 PDT
(In reply to Geoffrey Garen from comment #5)
> Comment on attachment 347398 [details]
> patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=347398&action=review
> 
> > Source/JavaScriptCore/dfg/DFGCFAPhase.cpp:147
> > +                    // the incoming value as if an invalidation point has occurred.
> 
> as if it could live past an invalidation point
> 
> > Source/JavaScriptCore/dfg/DFGCFAPhase.cpp:149
> > +                    // structure, and an InvalidationPoint no longer guarantees
> 
> It’s more like we’re violating the promise made by an invalidation point.
> The invalidating point itself still works.  We must conservatively act as if
> there’s an invalidation point because we do not pr Isley model them in osr
> entry metadata.

OSR happens at a point in time where if any watchpoints fired, we would have jettisoned the code block. Therefore, the most precise modeling of invalidation points at OSR is to say that they had happened. So, we are modeling them precisely in this patch.