137161 – DFG shouldn't insert store barriers when it has it on good authority that we're not storing a cell

Bug 137161 - DFG shouldn't insert store barriers when it has it on good authority that we're not storing a cell

Summary: DFG shouldn't insert store barriers when it has it on good authority that we'...

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	JavaScriptCore (show other bugs)
Version:	528+ (Nightly build)
Hardware:	All All

Importance:	P2 Normal
Assignee:	Filip Pizlo

URL:
Keywords:

Depends on:	137340
Blocks:
	Show dependency tree / graph

Reported:	2014-09-26 15:47 PDT by Filip Pizlo
Modified:	2014-10-13 17:43 PDT (History)
CC List:	8 users (show)

See Also:

Attachments
the patch (5.98 KB, patch) 2014-09-26 15:48 PDT, Filip Pizlo	mhahnenb: review+	Details \| Formatted Diff \| Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Filip Pizlo 2014-09-26 15:47:53 PDT

Patch forthcoming.

Comment 1 Filip Pizlo 2014-09-26 15:48:57 PDT

Created attachment 238742 [details]
the patch

Comment 2 Mark Hahnenberg 2014-09-26 15:51:03 PDT

Comment on attachment 238742 [details]
the patch

r=me!

Comment 3 Filip Pizlo 2014-09-26 15:54:06 PDT

Landed in http://trac.webkit.org/changeset/174025

Comment 4 Csaba Osztrogonác 2014-09-27 10:39:47 PDT

(In reply to comment #3)
> Landed in http://trac.webkit.org/changeset/174025

It made 3-4 performance tests crash everywhere:
- Apple Mountain Lion: https://build.webkit.org/builders/Apple%20MountainLion%20Release%20%28Perf%29/builds/10126
- Apple Mavericks: https://build.webkit.org/builders/Apple%20Mavericks%20Release%20%28Perf%29/builds/2663
- EFL: https://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2%20%28Perf%29/builds/3310

Comment 5 Filip Pizlo 2014-09-27 11:02:21 PDT

(In reply to comment #4)
> (In reply to comment #3)
> > Landed in http://trac.webkit.org/changeset/174025
> 
> It made 3-4 performance tests crash everywhere:
> - Apple Mountain Lion: https://build.webkit.org/builders/Apple%20MountainLion%20Release%20%28Perf%29/builds/10126
> - Apple Mavericks: https://build.webkit.org/builders/Apple%20Mavericks%20Release%20%28Perf%29/builds/2663
> - EFL: https://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2%20%28Perf%29/builds/3310

Ok, I will look. 

The consensus last us GC people chatted was that the barriers that this patch removed were merely masking the lack of barriers elsewhere. I'd rather fix those other barriers directly rather than rolling this out.

Comment 6 Mark Lam 2014-10-13 17:43:27 PDT

For the record, there was a follow up fix to this patch.  The fix was landed in r174121: <http://trac.webkit.org/r174121> by pizlo.