Summary: | Function allocation sinking shouldn't be performed on singleton functions | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Basile Clement <basile_clement> | ||||||||||
Component: | JavaScriptCore | Assignee: | Basile Clement <basile_clement> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, fpizlo, ggaren, mark.lam, msaboff | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Basile Clement
2015-04-24 15:42:55 PDT
Created attachment 251586 [details]
Patch
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 Created attachment 251587 [details]
Fix ChangeLog typos
(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. 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. (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. Created attachment 251772 [details]
Simplify ChangeLog entry
Created attachment 252172 [details]
Patch for landing
Comment on attachment 252172 [details] Patch for landing Clearing flags on attachment: 252172 Committed r183691: <http://trac.webkit.org/changeset/183691> All reviewed patches have been landed. Closing bug. |