Bug 130750

Summary: Repatch should support setters and plant calls to them directly
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, buildbot, ggaren, mark.lam, mhahnenberg, msaboff, oliver, ossy, rniwa, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 131336    
Bug Blocks: 129588    
Attachments:
Description Flags
almost done
none
the patch
ggaren: review+, buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
none
patch for landing none

Description Filip Pizlo 2014-03-25 15:46:09 PDT
Patch forthcoming.
Comment 1 Filip Pizlo 2014-04-07 21:41:42 PDT
Created attachment 228800 [details]
almost done
Comment 2 Filip Pizlo 2014-04-07 21:53:30 PDT
Created attachment 228802 [details]
the patch
Comment 3 Filip Pizlo 2014-04-07 22:00:15 PDT
This is a 12x speed-up on setter microbenchmarks.
Comment 4 Geoffrey Garen 2014-04-07 22:19:28 PDT
Comment on attachment 228802 [details]
the patch

r=me

Can you beef up setter.js to force a cached setter to miss the cache due to a redefined setter?
Comment 5 Filip Pizlo 2014-04-07 22:21:54 PDT
(In reply to comment #4)
> (From update of attachment 228802 [details])
> r=me
> 
> Can you beef up setter.js to force a cached setter to miss the cache due to a redefined setter?

Sure!
Comment 6 Build Bot 2014-04-07 23:07:48 PDT
Comment on attachment 228802 [details]
the patch

Attachment 228802 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5760488755429376

New failing tests:
platform/mac/fast/scrolling/scroll-iframe-latched-mainframe.html
platform/mac/fast/scrolling/scroll-select-latched-mainframe.html
js/regress/setter.html
platform/mac/fast/scrolling/scroll-div-latched-mainframe.html
Comment 7 Build Bot 2014-04-07 23:07:55 PDT
Created attachment 228808 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 8 Build Bot 2014-04-07 23:38:57 PDT
Comment on attachment 228802 [details]
the patch

Attachment 228802 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5040769945567232

New failing tests:
js/regress/setter.html
Comment 9 Build Bot 2014-04-07 23:39:02 PDT
Created attachment 228809 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 10 Build Bot 2014-04-07 23:56:04 PDT
Comment on attachment 228802 [details]
the patch

Attachment 228802 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6551012919738368

New failing tests:
platform/mac/fast/scrolling/scroll-iframe-latched-mainframe.html
platform/mac/fast/scrolling/scroll-select-latched-mainframe.html
js/regress/setter.html
platform/mac/fast/scrolling/scroll-div-latched-mainframe.html
Comment 11 Build Bot 2014-04-07 23:56:07 PDT
Created attachment 228810 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 12 Build Bot 2014-04-08 00:19:43 PDT
Comment on attachment 228802 [details]
the patch

Attachment 228802 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6604913685561344

New failing tests:
js/regress/setter.html
Comment 13 Build Bot 2014-04-08 00:19:47 PDT
Created attachment 228813 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 14 Filip Pizlo 2014-04-08 11:46:58 PDT
Created attachment 228862 [details]
patch for landing
Comment 15 Filip Pizlo 2014-04-08 12:40:40 PDT
Landed in http://trac.webkit.org/changeset/166945
Comment 16 Csaba Osztrogonác 2014-04-08 13:05:41 PDT
(In reply to comment #15)
> Landed in http://trac.webkit.org/changeset/166945
It broke 18 tests on 32 bit:
http://build.webkit.org/builders/Apple%20Mavericks%2032-bit%20JSC%20%28BuildAndTest%29/builds/248

** The following JSC stress test failures have been introduced:
	mozilla-tests.yaml/ecma/ExecutionContexts/10.1.6.js.mozilla-baseline
	mozilla-tests.yaml/ecma/ExecutionContexts/10.1.6.js.mozilla-dfg-eager-no-cjit-validate-phases
	mozilla-tests.yaml/ecma/Expressions/11.6.1-1.js.mozilla
	mozilla-tests.yaml/ecma/Expressions/11.6.1-1.js.mozilla-baseline
	mozilla-tests.yaml/ecma/Expressions/11.6.1-2.js.mozilla-baseline
	mozilla-tests.yaml/ecma/Expressions/11.6.2-1.js.mozilla-baseline
	mozilla-tests.yaml/ecma/Types/8.6.2.1-1.js.mozilla-baseline
	stress/setter.js.default
	stress/setter.js.no-llint
	stress/setter.js.always-trigger-copy-phase
	stress/setter.js.no-cjit-validate-phases
	regress/script-tests/assign-custom-setter-polymorphic.js.no-llint
	regress/script-tests/assign-custom-setter.js.no-llint
	regress/script-tests/assign-custom-setter.js.dfg-eager
	regress/script-tests/assign-custom-setter.js.dfg-eager-no-cjit-validate
	jsc-layout-tests.yaml/js/script-tests/for-in-cached.js.layout
	jsc-layout-tests.yaml/js/script-tests/for-in-cached.js.layout-no-llint
	jsc-layout-tests.yaml/js/script-tests/for-in-cached.js.layout-no-cjit

Results for JSC stress tests:
    18 failures found.
Comment 17 Filip Pizlo 2014-04-08 13:07:13 PDT
(In reply to comment #16)
> (In reply to comment #15)
> > Landed in http://trac.webkit.org/changeset/166945
> It broke 18 tests on 32 bit:
> http://build.webkit.org/builders/Apple%20Mavericks%2032-bit%20JSC%20%28BuildAndTest%29/builds/248
> 
> ** The following JSC stress test failures have been introduced:
>     mozilla-tests.yaml/ecma/ExecutionContexts/10.1.6.js.mozilla-baseline
>     mozilla-tests.yaml/ecma/ExecutionContexts/10.1.6.js.mozilla-dfg-eager-no-cjit-validate-phases
>     mozilla-tests.yaml/ecma/Expressions/11.6.1-1.js.mozilla
>     mozilla-tests.yaml/ecma/Expressions/11.6.1-1.js.mozilla-baseline
>     mozilla-tests.yaml/ecma/Expressions/11.6.1-2.js.mozilla-baseline
>     mozilla-tests.yaml/ecma/Expressions/11.6.2-1.js.mozilla-baseline
>     mozilla-tests.yaml/ecma/Types/8.6.2.1-1.js.mozilla-baseline
>     stress/setter.js.default
>     stress/setter.js.no-llint
>     stress/setter.js.always-trigger-copy-phase
>     stress/setter.js.no-cjit-validate-phases
>     regress/script-tests/assign-custom-setter-polymorphic.js.no-llint
>     regress/script-tests/assign-custom-setter.js.no-llint
>     regress/script-tests/assign-custom-setter.js.dfg-eager
>     regress/script-tests/assign-custom-setter.js.dfg-eager-no-cjit-validate
>     jsc-layout-tests.yaml/js/script-tests/for-in-cached.js.layout
>     jsc-layout-tests.yaml/js/script-tests/for-in-cached.js.layout-no-llint
>     jsc-layout-tests.yaml/js/script-tests/for-in-cached.js.layout-no-cjit
> 
> Results for JSC stress tests:
>     18 failures found.

I am looking.
Comment 18 Filip Pizlo 2014-04-08 13:17:09 PDT
(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > Landed in http://trac.webkit.org/changeset/166945
> > It broke 18 tests on 32 bit:
> > http://build.webkit.org/builders/Apple%20Mavericks%2032-bit%20JSC%20%28BuildAndTest%29/builds/248
> > 
> > ** The following JSC stress test failures have been introduced:
> >     mozilla-tests.yaml/ecma/ExecutionContexts/10.1.6.js.mozilla-baseline
> >     mozilla-tests.yaml/ecma/ExecutionContexts/10.1.6.js.mozilla-dfg-eager-no-cjit-validate-phases
> >     mozilla-tests.yaml/ecma/Expressions/11.6.1-1.js.mozilla
> >     mozilla-tests.yaml/ecma/Expressions/11.6.1-1.js.mozilla-baseline
> >     mozilla-tests.yaml/ecma/Expressions/11.6.1-2.js.mozilla-baseline
> >     mozilla-tests.yaml/ecma/Expressions/11.6.2-1.js.mozilla-baseline
> >     mozilla-tests.yaml/ecma/Types/8.6.2.1-1.js.mozilla-baseline
> >     stress/setter.js.default
> >     stress/setter.js.no-llint
> >     stress/setter.js.always-trigger-copy-phase
> >     stress/setter.js.no-cjit-validate-phases
> >     regress/script-tests/assign-custom-setter-polymorphic.js.no-llint
> >     regress/script-tests/assign-custom-setter.js.no-llint
> >     regress/script-tests/assign-custom-setter.js.dfg-eager
> >     regress/script-tests/assign-custom-setter.js.dfg-eager-no-cjit-validate
> >     jsc-layout-tests.yaml/js/script-tests/for-in-cached.js.layout
> >     jsc-layout-tests.yaml/js/script-tests/for-in-cached.js.layout-no-llint
> >     jsc-layout-tests.yaml/js/script-tests/for-in-cached.js.layout-no-cjit
> > 
> > Results for JSC stress tests:
> >     18 failures found.
> 
> I am looking.

Does not repro in release for me.  Trying debug now...
Comment 19 Filip Pizlo 2014-04-08 13:23:01 PDT
(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > (In reply to comment #15)
> > > > Landed in http://trac.webkit.org/changeset/166945
> > > It broke 18 tests on 32 bit:
> > > http://build.webkit.org/builders/Apple%20Mavericks%2032-bit%20JSC%20%28BuildAndTest%29/builds/248
> > > 
> > > ** The following JSC stress test failures have been introduced:
> > >     mozilla-tests.yaml/ecma/ExecutionContexts/10.1.6.js.mozilla-baseline
> > >     mozilla-tests.yaml/ecma/ExecutionContexts/10.1.6.js.mozilla-dfg-eager-no-cjit-validate-phases
> > >     mozilla-tests.yaml/ecma/Expressions/11.6.1-1.js.mozilla
> > >     mozilla-tests.yaml/ecma/Expressions/11.6.1-1.js.mozilla-baseline
> > >     mozilla-tests.yaml/ecma/Expressions/11.6.1-2.js.mozilla-baseline
> > >     mozilla-tests.yaml/ecma/Expressions/11.6.2-1.js.mozilla-baseline
> > >     mozilla-tests.yaml/ecma/Types/8.6.2.1-1.js.mozilla-baseline
> > >     stress/setter.js.default
> > >     stress/setter.js.no-llint
> > >     stress/setter.js.always-trigger-copy-phase
> > >     stress/setter.js.no-cjit-validate-phases
> > >     regress/script-tests/assign-custom-setter-polymorphic.js.no-llint
> > >     regress/script-tests/assign-custom-setter.js.no-llint
> > >     regress/script-tests/assign-custom-setter.js.dfg-eager
> > >     regress/script-tests/assign-custom-setter.js.dfg-eager-no-cjit-validate
> > >     jsc-layout-tests.yaml/js/script-tests/for-in-cached.js.layout
> > >     jsc-layout-tests.yaml/js/script-tests/for-in-cached.js.layout-no-llint
> > >     jsc-layout-tests.yaml/js/script-tests/for-in-cached.js.layout-no-cjit
> > > 
> > > Results for JSC stress tests:
> > >     18 failures found.
> > 
> > I am looking.
> 
> Does not repro in release for me.  Trying debug now...

Oh yuck!!  That assertion probably affects 64-bit too.  It's a dumb assertion, will land fix shortly...
Comment 20 Filip Pizlo 2014-04-08 13:26:21 PDT
(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > (In reply to comment #16)
> > > > (In reply to comment #15)
> > > > > Landed in http://trac.webkit.org/changeset/166945
> > > > It broke 18 tests on 32 bit:
> > > > http://build.webkit.org/builders/Apple%20Mavericks%2032-bit%20JSC%20%28BuildAndTest%29/builds/248
> > > > 
> > > > ** The following JSC stress test failures have been introduced:
> > > >     mozilla-tests.yaml/ecma/ExecutionContexts/10.1.6.js.mozilla-baseline
> > > >     mozilla-tests.yaml/ecma/ExecutionContexts/10.1.6.js.mozilla-dfg-eager-no-cjit-validate-phases
> > > >     mozilla-tests.yaml/ecma/Expressions/11.6.1-1.js.mozilla
> > > >     mozilla-tests.yaml/ecma/Expressions/11.6.1-1.js.mozilla-baseline
> > > >     mozilla-tests.yaml/ecma/Expressions/11.6.1-2.js.mozilla-baseline
> > > >     mozilla-tests.yaml/ecma/Expressions/11.6.2-1.js.mozilla-baseline
> > > >     mozilla-tests.yaml/ecma/Types/8.6.2.1-1.js.mozilla-baseline
> > > >     stress/setter.js.default
> > > >     stress/setter.js.no-llint
> > > >     stress/setter.js.always-trigger-copy-phase
> > > >     stress/setter.js.no-cjit-validate-phases
> > > >     regress/script-tests/assign-custom-setter-polymorphic.js.no-llint
> > > >     regress/script-tests/assign-custom-setter.js.no-llint
> > > >     regress/script-tests/assign-custom-setter.js.dfg-eager
> > > >     regress/script-tests/assign-custom-setter.js.dfg-eager-no-cjit-validate
> > > >     jsc-layout-tests.yaml/js/script-tests/for-in-cached.js.layout
> > > >     jsc-layout-tests.yaml/js/script-tests/for-in-cached.js.layout-no-llint
> > > >     jsc-layout-tests.yaml/js/script-tests/for-in-cached.js.layout-no-cjit
> > > > 
> > > > Results for JSC stress tests:
> > > >     18 failures found.
> > > 
> > > I am looking.
> > 
> > Does not repro in release for me.  Trying debug now...
> 
> Oh yuck!!  That assertion probably affects 64-bit too.  It's a dumb assertion, will land fix shortly...

Fixed in http://trac.webkit.org/changeset/166952