Closed Bug 861832 Opened 9 years ago Closed 8 years ago

IonMonkey: Fix raytrace regression due to 768288

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: h4writer, Unassigned)

References

Details

We took a hit on raytrace to enable loop inlining.

The issue is as following:
- func1 inlines func2
- func2 takes enormously long to compile (due to inlining a lot of scripts)
- func1 get's called in a loop. 

So when we first hit func1, func1 gets scheduled for compilation on background thread. The loop keeps going using the slower BC/interpreter. Only when func1 is compiled, we can execute using IM. So every loop we loose the time difference between IM and BC/Interpreter.

Solutions:
1) We want to have an IM version really quick. So I was thinking that we could adjust IM to schedule 2 compiles. CompileV1, would be the quick one and doesn't inline anything. CompileV2 would be the full version. CompileV1 wouldn't take long to compile and we would use it till the time CompileV2 would be ready. We even could make the CompileV1 have a higher priority. I.e. always compiling V1 first, before compiling a V2 of any script.

2) Atm the maxInlineByteCode is tested on a local base. If we could postpone inlining till after IonBuilder (I know this a big change). Than we would know which scripts wants to get inline and can decide which one to inline more globally. I.e. making sure inlining keeps the script size of the inlined functions under inlinedmaxInlineByteCode... And decide in a global way which one not to inline.

3) Less invasive as previous solutions. We could keep a totalBytecode in IonScript, that would be the bytecode size of all inlined scripts. This wouldn't disable getting scripts big due to inlining way to much, but we wouldn't inline those atleast ...
Blocks: 768288
Any updates here?
Sorry about the delay. Somehow I must have overlooked your question. But the original patch of bug 768288 was backed out due to regressions and a less invasive version was posted. So we didn't have this regression.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.