Closed Bug 728045 Opened 12 years ago Closed 12 years ago

IonMonkey: ion::GetPcScript should not depend on the snapshot.

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: nbp, Assigned: nbp)

References

(Blocks 1 open bug)

Details

(Whiteboard: [ion:t])

Attachments

(5 files, 1 obsolete file)

Here are the number of times the message is seen per tests.  This error is the major cause of bailout.

   1620  bailing from bytecode: nop, MIR: constant, LIR: osipoint (jit-test/tests/ion/throw.js:1)
    758  bailing from bytecode: nop, MIR: constant, LIR: osipoint (jit-test/tests/ion/getelem-bounds-hoist.js:75)
    163  bailing from bytecode: nop, MIR: constant, LIR: osipoint (jit-test/tests/ion/InlineAddVTypeMonitor.js:10)
     24  bailing from bytecode: nop, MIR: constant, LIR: osipoint (jit-test/tests/ion/bug716853.js:7)
     18  bailing from bytecode: nop, MIR: constant, LIR: osipoint (jit-test/tests/ion/lambda.js:18)
      8  bailing from bytecode: nop, MIR: constant, LIR: osipoint (jit-test/tests/ion/invalidation/framedescriptors.js:3)
      8  bailing from bytecode: nop, MIR: constant, LIR: osipoint (jit-test/tests/ion/inlining/inline-callarg-ubench-no-double2.js:7)
      2  bailing from bytecode: nop, MIR: constant, LIR: osipoint (jit-test/tests/ion/invalidation/outofline.js:13)
This patch separate the bailout from getPcScript function calls.
Most of the path reported here are caused by calls to getPcScript which are likely to be called from TypeMonitor functions.

getPcScript is considered as a slow function as it implies reading a snapshot and the bytecode to recover the PC and the script.

To test run with IONFLAGS=all … 2>&1 | grep -c "Recover PC"
Attachment #599002 - Flags: review?(christopher.leary)
Attachment #599002 - Flags: review?(christopher.leary) → review+
https://hg.mozilla.org/projects/ionmonkey/rev/242a9051f7e9 (sorry, I forgot to add cdleary as a reviewer in the commit message)

The bug should not be closed yet.
Ok, I checked the number of times we call GetPcScript function for each sunspider tests, and the number of times we have to iterate on inlined frames.

Currently, we have the following figures, other sunspider tests have 0 use of getPcScript.  And no test use inlined frame when calling getPcScript.

3d-cube.js: 4615
3d-morph.js: 10
3d-raytrace.js: 29207
access-nsieve.js: 38
bitops-nsieve-bits.js: 7
crypto-aes.js: 8
crypto-md5.js: 9
crypto-sha1.js: 9
date-format-xparb.js: 19879
math-spectral-norm.js: 15
string-base64.js: 32453
string-fasta.js: 175005
string-tagcloud.js: 4993

GetPcScript on non-inlined frames implies:
 - Reading the top frame. (stack)
 - Reading the previous frame calleeToken. (stack)
 - Read SafepointIndex. (ionscript tail)
 - Read Safepoint. (ionscript tail)
 - Read OsiIndex. (ionscript tail)
 - Return Snapshot fields. (ionscript tail)
Summary: IonMonkey: Bailing out from bytecode: nop, MIR: constant, LIR: osipoint (8 tests in jit-test/ion) → IonMonkey: ion::GetPcScript accounts for 1/3 of the time of sunspider/string-fasta.
Assignee: general → nicolas.b.pierron
Where is GetPcScript getting called from?
(In reply to David Anderson [:dvander] from comment #4)
> Where is GetPcScript getting called from?

js::GetElement (cumul 64%, self 8%)
|- js::types::TypeScript::GetPcScript (cumul 31%, < 1%)
|  |- js::ion::GetPcScript (cumul 30%, self 2%)
|- js::types::TypeMonitorResult (cumul 8%, self 4%)
Aha, this might just be fixed by bug 725357 then - I forgot to review that, I'll do it ASAP.
As the only instance of this bug was under GetElement, I flag this bug as a duplicate of bug 725357 which move the monitor result outside of GetElement function and inline it as a typeset check and a monitor result call in case of bailout.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → DUPLICATE
When GetPcScript is called from an ool path under a callVM, the function of the inlined frame is not easily available (need to recover it from the live register pushed on the stack)

GetPcScript would need to be updated with a mechanism which does not rely on iterating over the snapshot, and which maps the displacement to the script & pc of the last (inlined?) frame.

This can be done by adding one more displacement mapping to the IonScript, as already done for safepoint, and osipoint.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Summary: IonMonkey: ion::GetPcScript accounts for 1/3 of the time of sunspider/string-fasta. → IonMonkey: ion::GetPcScript should not depend on the snapshot.
CompactMap is a way to save space while providing a map algorithm which iterates on a few elements for the lookup.

It save up to 1.88 / 3.33 times the space used by a naive implementation on 32bits / 64bits architecture by using delta-compression of offsets.  It is worth using it for vectors larger than 24 / 8 elements.

It is used to store pc & script for GetPcScript because this Pc & Script information is likely to be used in the future to profile & debug the original Javascript.
Attachment #601490 - Flags: review?(dvander)
Add more comments to CompactMap.
Fix pc & script registration in CodeGenerator (each time we do a markSafepointAt)
Attachment #601490 - Attachment is obsolete: true
Attachment #601889 - Flags: review?(dvander)
Attachment #601490 - Flags: review?(dvander)
This patch looks really complicated to me and I think we need better justification for it. There are two bugs we want to fix:

 (1) There are hot paths which call GetPcScript.
 (2) GetPcScript is slow.

The first is a very important target, since those are paths we need to optimize anyway. In those cases we should not call GetPcScript. That leaves the other paths which might need to call it.

Optimizing GetPcScript for those cases is a good idea (measurements backing it up (sans new Array/String) would be nice, I haven't seen any yet), but I think we can do it without a new data structure that introduces 350+ LoC. There are two reasons GetPcScript is slow:

  (1) We have to find the OSI point corresponding to the return address.
  (2) We have to iterate over the snapshot.

(2) is really easy to solve. Snapshots already have almost all the information we need. If inlineFrameCount == 0, we can just use the CalleeToken's script, and the snapshot's pc. If inlineFrameCount == 0, we can add the inlined script index and pc offset into the beginning of the snapshot, which would generally be 2-4 bytes.

(1) Is harder, but here's a few ideas:

 (a) A very simple (return address) -> (return address, script, pc) cache that is stored in the IonCompartment. It could have a fixed number of entries and just evict on mismatch.

 (b) If we're inlining, force CallGeneric/CallNative to store the inlined script index and pc offset into the JSRuntime (and we'd save old ones in IonActivation). 

 (c) Measure to see if just #2 is enough to stave off needing more work.
er, "If inlineFrameCount == 0, we can add the inlined script" that should be != 0
Based on this graph, we need to store efficiently both the for small numbers and also for large numbers.

The highest points at the end of the plots are v8 benchmarks which involve extremely large functions, and IonMonkey currently compiled them more than 14 times each which is a bug ! The largest one (920 mapping registered) is likely to be regexp.js:1324 runBlock11.
Take care of the log scale on the X axe.

I truncated this graph to avoid showing v8bench recompilation bugs even if the mapping on the compiled script which has 920 mapping registered goes to (3k/10k on x86/x64)
This contains the most important use cases of GetPcScript in all benchmarks.  
js::GetScopeName is mostly used in v8 benchmarks and would be removed with Bug 659577.
js::SetObjectElement case should be removed with Bug 730836.

js::ModValues is likely to be a badly typed in the MIR, and calls MonitorOverflow (?)
js::AddValues calls MonitorString.
js::str_split is creating a new String and attach a type to it.
Bug 794679 provide a cache for GetPcScript, which makes this issue less important.  Flag it as RESOLVED WONTFIX.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.