Bug 144166 – Function allocation sinking shouldn't be performed on singleton functions

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Basile Clement]

Patch forthcoming.

[Basile Clement]

Created attachment 251586 [details]
Patch

[Geoffrey Garen]

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]

Created attachment 251587 [details]
Fix ChangeLog typos

[Basile Clement]

(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 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]

(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]

Created attachment 251772 [details]
Simplify ChangeLog entry

[Basile Clement]

Created attachment 252172 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 252172 [details]
Patch for landing

Clearing flags on attachment: 252172

Committed r183691: <http://trac.webkit.org/changeset/183691>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.