Bug 129170

Summary: Add extra space to op_call and related opcodes
Product: WebKit Reporter: Oliver Hunt <oliver>
Component: New BugsAssignee: Oliver Hunt <oliver>
Status: RESOLVED FIXED    
Severity: Normal CC: fpizlo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch mark.lam: review+

Description Oliver Hunt 2014-02-21 14:23:16 PST
Add extra space to op_call and related opcodes
Comment 1 Oliver Hunt 2014-02-21 14:25:16 PST
Created attachment 224908 [details]
Patch
Comment 2 Mark Lam 2014-02-21 14:28:26 PST
Comment on attachment 224908 [details]
Patch

r=me
Comment 3 Daniel Bates 2014-02-21 14:29:31 PST
Comment on attachment 224908 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=224908&action=review

> Source/JavaScriptCore/ChangeLog:9
> +        slot to the op_call instructions, and refactoring to make simlar

Nit: simlar => similar
Comment 4 Daniel Bates 2014-02-21 14:30:06 PST
Comment on attachment 224908 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=224908&action=review

> Source/JavaScriptCore/ChangeLog:10
> +        changes easier in future.

Nit: "in future" => "in the future"
Comment 5 Oliver Hunt 2014-02-21 14:32:19 PST
Committed r164503: <http://trac.webkit.org/changeset/164503>
Comment 6 Filip Pizlo 2014-02-21 18:12:16 PST
Why?

Did you test performance?  Inlining is based on bytecode size, and you just made everything larger.
Comment 7 Oliver Hunt 2014-02-21 20:26:11 PST
(In reply to comment #6)
> Why?
> 
> Did you test performance?  Inlining is based on bytecode size, and you just made everything larger.

Because call_var_args needs an extra argument, i didn't think about the performance impact because i forgot that inlining was based on # of operands rather than # of opcodes.

I'll see if i can fix this, or alternatively just remove the requirement that call_var_args be the same size as all the other calls.
Comment 8 Filip Pizlo 2014-02-21 21:12:09 PST
(In reply to comment #7)
> (In reply to comment #6)
> > Why?
> > 
> > Did you test performance?  Inlining is based on bytecode size, and you just made everything larger.
> 
> Because call_var_args needs an extra argument, i didn't think about the performance impact because i forgot that inlining was based on # of operands rather than # of opcodes.
> 
> I'll see if i can fix this, or alternatively just remove the requirement that call_var_args be the same size as all the other calls.

I think it's enough to just measure it and make sure that nothing broke
Comment 9 Oliver Hunt 2014-02-21 21:38:59 PST
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > Why?
> > > 
> > > Did you test performance?  Inlining is based on bytecode size, and you just made everything larger.
> > 
> > Because call_var_args needs an extra argument, i didn't think about the performance impact because i forgot that inlining was based on # of operands rather than # of opcodes.
> > 
> > I'll see if i can fix this, or alternatively just remove the requirement that call_var_args be the same size as all the other calls.
> 
> I think it's enough to just measure it and make sure that nothing broke

kk
Comment 10 Oliver Hunt 2014-02-22 13:26:39 PST
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > Why?
> > > 
> > > Did you test performance?  Inlining is based on bytecode size, and you just made everything larger.
> > 
> > Because call_var_args needs an extra argument, i didn't think about the performance impact because i forgot that inlining was based on # of operands rather than # of opcodes.
> > 
> > I'll see if i can fix this, or alternatively just remove the requirement that call_var_args be the same size as all the other calls.
> 
> I think it's enough to just measure it and make sure that nothing broke

I don't see a regression under jsc on my MBP.  Just about to run the DRT hosted version.