Status

()

defect
RESOLVED DUPLICATE of bug 581263
10 years ago
9 years ago

People

(Reporter: sayrer, Assigned: sayrer)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Assignee

Description

10 years ago
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.
Assignee

Comment 1

10 years ago
three labels down, two to go
Assignee

Comment 2

10 years ago
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" :)
Attachment #407388 - Attachment description: first bit → first bit (WIP)
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
I mean "putting too many params into memory"....

/be
Assignee

Comment 5

10 years ago
(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.
(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.
IA32 is register-impoverished, x64 not. ARMs vary, I haven't cracked the manual. Anyone know off the top of their head?

/be
Assignee

Comment 8

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

With this patch, SunSpider is unchanged on my linux x64 system.
When you see js::Invoke after bug 581263 you will shed a single tear of joy.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 581263
You need to log in before you can comment on or make changes to this bug.