Bug 129170 – Add extra space to op_call and related opcodes

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Oliver Hunt]

Add extra space to op_call and related opcodes

[Oliver Hunt]

Created attachment 224908 [details]
Patch

[Mark Lam]

Comment on attachment 224908 [details]
Patch

r=me

[Daniel Bates]

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

[Daniel Bates]

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"

[Oliver Hunt]

Committed r164503: <http://trac.webkit.org/changeset/164503>

[Filip Pizlo]

Why?

Did you test performance?  Inlining is based on bytecode size, and you just made everything larger.

[Oliver Hunt]

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

[Filip Pizlo]

(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

[Oliver Hunt]

(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

[Oliver Hunt]

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