Bug 75911

Summary:

REGRESSION: d3 Bullet Charts demo doesn't work (call with argument assignment is broken)

Product:

WebKit

Reporter:

Geoffrey Garen <ggaren>

Component:

WebKit Misc.

Assignee:

Geoffrey Garen <ggaren>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

fpizlo

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	fpizlo: review+

Description Geoffrey Garen 2012-01-09 16:41:16 PST

http://mbostock.github.com/d3/ex/bullet.html

Displays no chart on ToT, but it works on Safari 5.1.2.

Comment 1 Geoffrey Garen 2012-01-09 16:51:15 PST

<rdar://problem/10650878>

Comment 2 Geoffrey Garen 2012-01-09 17:22:28 PST

Created attachment 121766 [details]
Patch

Comment 3 Filip Pizlo 2012-01-09 17:30:17 PST

Comment on attachment 121766 [details]
Patch

Yay!  r=me

Comment 4 Filip Pizlo 2012-01-09 17:30:30 PST

Any performance impact?

Comment 5 Geoffrey Garen 2012-01-10 11:03:59 PST

> Any performance impact?

Bencher says 1.002x worse on SunSpider and v8 -- seems reasonable to call that "no change".

Bytecode generation for f.apply is definitely worse by one op_mov. This is theoretically fixable with more robust optimization information in the AST, but I think that's probably the wrong direction -- long-term, we just want to compile f.apply in the DFG, which will elide the op_mov.

Comment 6 Filip Pizlo 2012-01-10 11:46:03 PST

(In reply to comment #5)
> > Any performance impact?
> 
> Bencher says 1.002x worse on SunSpider and v8 -- seems reasonable to call that "no change".
> 
> Bytecode generation for f.apply is definitely worse by one op_mov. This is theoretically fixable with more robust optimization information in the AST, but I think that's probably the wrong direction -- long-term, we just want to compile f.apply in the DFG, which will elide the op_mov.

Agree!

Comment 7 Geoffrey Garen 2012-01-11 16:14:46 PST

Committed r104762: <http://trac.webkit.org/changeset/104762>