523459 - clean up js_Invoke

Status: RESOLVED DUPLICATE of bug 581263

(Core :: JavaScript Engine, defect)

Description

[Robert Sayre]

I found js_Invoke pretty difficult to understand. It seems to me that we have some C++ tools we can use to clean it up.

My plan is to untangle the gotos by decomposing to functions with too many parameters, using RAII to handle "goto bad", and then consolidating the function parameters into some data structures.

Comment 1

[Robert Sayre]

Attachment: first bit (WIP)

three labels down, two to go

Comment 2

[Robert Sayre]

Comment on attachment 407388 [details] [diff] [review]
first bit (WIP)

marking WIP. do not worry that I will stick with a function named "js_Invoke2" :)

Comment 3

[Brendan Eich [:brendan]]

It's important to measure code size and cycles too. Clean and fast is good. Clean and slow, not good. It isn't obvious you were gonna focus on perf and codesize in addition to making it easier to understand, so I thought I'd throw this in. There are traps here: putting parameters into memory won't hurt on IA32 but will on better architectures.

/be

Comment 4

[Brendan Eich [:brendan]]

I mean "putting too many params into memory"....

/be

Comment 5

[Robert Sayre]

(In reply to comment #3)
> It's important to measure code size and cycles too. Clean and fast is good.
> Clean and slow, not good.

Who could argue with that? One of the tests here is to see if this actually hurts. Shark always says most things we do in js_Invoke are close to free. Maybe there are cases I haven't seen, or maybe I will make them non-free. It will be interesting to see if slowness shows up. 

> putting parameters into memory won't hurt on IA32
> but will on better architectures.

What are the better architectures? x64 or ARM? I should measure on those as well. It's not clear to me that a static inline function will encounter this hazard, so the first patch here seems ok, but maybe this comment was (rightly) addressing threats of further abstraction.

Comment 6

[David Mandelin [:dmandelin]]

(In reply to comment #5)
> (In reply to comment #3)
> > putting parameters into memory won't hurt on IA32
> > but will on better architectures.
> 
> What are the better architectures? x64 or ARM? I should measure on those as
> well. It's not clear to me that a static inline function will encounter this
> hazard, so the first patch here seems ok, but maybe this comment was (rightly)
> addressing threats of further abstraction.

My recent work staring at disassembled FF/Win32 code indicates that MSVC is pretty smart about inlining, parameter passing, and interprocedural register allocation (to the extent of creating special calling conventions for functions or non-inlined parts thereof), so I highly doubt we have anything to worry about there. GCC is less smart but I think it can be tormented into doing the right thing without undue difficulty.

Comment 7

[Brendan Eich [:brendan]]

IA32 is register-impoverished, x64 not. ARMs vary, I haven't cracked the manual. Anyone know off the top of their head?

/be

Comment 8

[Robert Sayre]

Comment on attachment 407388 [details] [diff] [review]
first bit (WIP)

With this patch, SunSpider is unchanged on my linux x64 system.

Comment 9

[Luke Wagner [:luke]]

When you see js::Invoke after bug 581263 you will shed a single tear of joy.