RESOLVED FIXED 144166
Function allocation sinking shouldn't be performed on singleton functions
https://bugs.webkit.org/show_bug.cgi?id=144166
Summary Function allocation sinking shouldn't be performed on singleton functions
Basile Clement
Reported 2015-04-24 15:42:55 PDT
Patch forthcoming.
Attachments
Patch (2.86 KB, patch)
2015-04-24 16:21 PDT, Basile Clement
ggaren: review+
ggaren: commit-queue-
Fix ChangeLog typos (2.86 KB, patch)
2015-04-24 16:25 PDT, Basile Clement
fpizlo: review+
Simplify ChangeLog entry (2.80 KB, patch)
2015-04-27 12:54 PDT, Basile Clement
no flags
Patch for landing (2.96 KB, patch)
2015-05-01 14:46 PDT, Basile Clement
no flags
Basile Clement
Comment 1 2015-04-24 16:21:11 PDT
Geoffrey Garen
Comment 2 2015-04-24 16:23:31 PDT
Comment on attachment 251586 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=251586&action=review r=me cq- for some ChangeLog edits. > Source/JavaScriptCore/ChangeLog:8 > + Function allocation usually are free of any other side effects, but allocation => allocations > Source/JavaScriptCore/ChangeLog:18 > + to worry about its watchpoint, allowing to use allowing to use => allowing us to use
Basile Clement
Comment 3 2015-04-24 16:25:22 PDT
Created attachment 251587 [details] Fix ChangeLog typos
Basile Clement
Comment 4 2015-04-24 16:26:35 PDT
(In reply to comment #2) > Comment on attachment 251586 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=251586&action=review > > r=me Thanks! > > cq- for some ChangeLog edits. > > > Source/JavaScriptCore/ChangeLog:8 > > + Function allocation usually are free of any other side effects, but > > allocation => allocations > > > Source/JavaScriptCore/ChangeLog:18 > > + to worry about its watchpoint, allowing to use > > allowing to use => allowing us to use Fixed those in a new patch.
Filip Pizlo
Comment 5 2015-04-27 12:28:42 PDT
Comment on attachment 251587 [details] Fix ChangeLog typos View in context: https://bugs.webkit.org/attachment.cgi?id=251587&action=review Holding off cq because the ChangeLog comment could be refined. > Source/JavaScriptCore/ChangeLog:14 > + watchpoints invalidating code that depends on it being a singleton). According > + to fpizlo, this happens to be sinkable right now, but things may not stay that > + way in the future (e.g. it would not be the case for CreateActivation), and > + fixing this now would help prevent hard-to-find bugs later in that case. What won't stay that way in the future? That singleton function allocations are sinkable? I think it's enough to say that we don't want to sink code that has effects if we haven't done the work to prove that the effect wasn't observed. Object allocation sinking phase does no such analysis because it assumes that the act of allocating is not effectful.
Basile Clement
Comment 6 2015-04-27 12:41:45 PDT
(In reply to comment #5) > Comment on attachment 251587 [details] > Fix ChangeLog typos > > View in context: > https://bugs.webkit.org/attachment.cgi?id=251587&action=review > > Holding off cq because the ChangeLog comment could be refined. > > > Source/JavaScriptCore/ChangeLog:14 > > + watchpoints invalidating code that depends on it being a singleton). According > > + to fpizlo, this happens to be sinkable right now, but things may not stay that > > + way in the future (e.g. it would not be the case for CreateActivation), and > > + fixing this now would help prevent hard-to-find bugs later in that case. > > What won't stay that way in the future? That singleton function allocations > are sinkable? > > I think it's enough to say that we don't want to sink code that has effects > if we haven't done the work to prove that the effect wasn't observed. > Object allocation sinking phase does no such analysis because it assumes > that the act of allocating is not effectful. OK, will simplify that.
Basile Clement
Comment 7 2015-04-27 12:54:35 PDT
Created attachment 251772 [details] Simplify ChangeLog entry
Basile Clement
Comment 8 2015-05-01 14:46:05 PDT
Created attachment 252172 [details] Patch for landing
WebKit Commit Bot
Comment 9 2015-05-01 15:34:49 PDT
Comment on attachment 252172 [details] Patch for landing Clearing flags on attachment: 252172 Committed r183691: <http://trac.webkit.org/changeset/183691>
WebKit Commit Bot
Comment 10 2015-05-01 15:34:53 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.