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)
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: nbp, Assigned: nbp)
References
(Blocks 1 open bug)
Details
(Whiteboard: [ion:t])
Attachments
(5 files, 1 obsolete file)
1.12 KB,
patch
|
cdleary
:
review+
|
Details | Diff | Splinter Review |
41.51 KB,
patch
|
Details | Diff | Splinter Review | |
4.21 KB,
image/png
|
Details | |
5.10 KB,
image/png
|
Details | |
9.48 KB,
text/plain
|
Details |
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)
Assignee | ||
Comment 1•12 years ago
|
||
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)
Updated•12 years ago
|
Attachment #599002 -
Flags: review?(christopher.leary) → review+
Assignee | ||
Comment 2•12 years ago
|
||
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.
Assignee | ||
Comment 3•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
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 | ||
Updated•12 years ago
|
Assignee: general → nicolas.b.pierron
Where is GetPcScript getting called from?
Assignee | ||
Comment 5•12 years ago
|
||
(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.
Assignee | ||
Comment 7•12 years ago
|
||
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
Assignee | ||
Comment 8•12 years ago
|
||
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.
Assignee | ||
Updated•12 years ago
|
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.
Assignee | ||
Comment 9•12 years ago
|
||
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)
Assignee | ||
Comment 10•12 years ago
|
||
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
Assignee | ||
Comment 13•12 years ago
|
||
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.
Assignee | ||
Comment 14•12 years ago
|
||
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)
Assignee | ||
Comment 15•12 years ago
|
||
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.
Updated•12 years ago
|
Attachment #601889 -
Flags: review?(dvander)
Updated•12 years ago
|
Whiteboard: [ion:t]
Assignee | ||
Comment 16•12 years ago
|
||
Bug 794679 provide a cache for GetPcScript, which makes this issue less important. Flag it as RESOLVED WONTFIX.
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•