813425 - IonMonkey: Octane, pdfjs.js:16934: Invalidating due to shape guard failure.

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

When Bug 807464 and Bug 807443 are fixed (work-around), this function is invalidated 11 times, and the last 9 are prefixed with "Invalidating due to shape guard failure".

Investigate why this function is invalidated and potential way to fix this.

Comment 1

[Nicolas B. Pierron [:nbp]]

Currently, the function line 16934 is recompiled 150 times where only ~ 30 are expected.
This guard shape appear on “program.properties” property, I still have to find why we are adding this on a property which is allocated in the same function.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: PdfJS Octane benchmark diff to show expected perfs.

The recompilations are induced by the 114 guard shape failures which are raised when the code is reading the property lenIV out of the privateData object in Type1Parser_extractFontProgram.  
The reason why we have so many failures is caused by a loop which is adding/setting properties to the privateData object in a random order (depends on the input).

Adding all properties in the privateData object when it is created, gives a 2.4% speedup to the PdfJS benchmark.

Without changing the order of shapes, we can see that all accesses to lenIV are sharing the same parent shape having a fixed slot for the lenIV property, and thus prevent the number of recompilation of this big function.  This would not save us from adding / setting plenty of IC for properties, and so we should not expect as much performances.

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: Prevent JM cache sniffing when they are not hot enough.

(Shame on me, I did not notice that we were setting a flag when we failed a shape guard, and I was to focus on sharing shape guard that I forgot to look around …)

This patch fallback to a getPropertyCache implementation, which will likely overflow after being re-entered a hundred of times.  As the lenIV property is not  used a lot, based on the input which basically terminate by the evaluation of the lenIV path, there is no need to look for extreme performance gain by handling common inherited shape.

This patch provides a 2.7% speed-up of PdfJS octane benchmark. (why above 2.4%, probably some benchmark noise ?!)

NB: If we are not resetting the Baseline ICs, we might remove this restriction in the future since we will aggregate all visited types.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 696747 [details] [diff] [review]
Prevent JM cache sniffing when they are not hot enough.

Review of attachment 696747 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is what I tried in bug 784127, but it regressed JSNES. Can you check how this patch affects that benchmark?

With the baseline compiler we definitely don't want to purge caches and can hopefully make this all more robust.

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: WIP: Use cache for shape guards.

This patch add a shape guard IC currently for all shape guards, which handles shapes with a common parent, enough to abstract over all slot access used.

This patch does not yet mark the information stored in the Bytecode structure, as well as the jsid stored in the MGuardShape access structure.

Currently, this patch performs badly on a few benchmark because of another bug which cause multiple shapes to be allocated for identical objects.  Which makes many bytecode appear as polymorphic, where JM ICs would only have reported a monomorphic case.

Comment 6

[Nicolas B. Pierron [:nbp]]

The latest patch attach to this bug show the following performances (see Bug 835306 for details):

 ext | num |  Caf  |  Car  |  Cam  |  CAf  |  CAr  |  CAm  |  caf  |  car  |  cam  |  cAf  |  cAr  |  cAm  |
 10  |  1  | 465.9 | 390.4 | 387.0 | 6.934 | 6.798 | 6.439 | 374.1 | 386.8 | 400.2 | 8.866 | 7.203 | 7.068 |
 10  |  2  | 383.3 | 368.9 | 376.7 | 9.032 | 7.247 | 9.434 | 379.3 | 372.9 | 367.4 | 31.37 | 34.09 | 33.44 |
 10  |  4  | 382.1 | 374.0 | 372.7 | 9.182 | 9.323 | 8.105 | 369.7 | 367.2 | 368.1 | 42.97 | 44.33 | 41.68 |
 10  |  6  | 367.7 | 369.4 | 368.3 | 9.781 | 9.581 | 9.301 | 374.2 | 379.7 | 368.8 | 51.91 | 58.09 | 58.08 |
 10  |  8  | 373.2 | 369.8 | 369.4 | 9.873 | 7.626 | 10.42 | 378.3 | 375.9 | 373.2 | 76.72 | 65.88 | 64.57 |
 10  | 12  | 378.3 | 381.3 | 380.1 | 11.01 | 9.340 | 14.28 | 388.8 | 385.4 | 379.7 | 82.88 | 81.78 | 75.28 |
 10  | 16  | 396.7 | 389.6 | 388.3 | 10.33 | 11.39 | 16.56 | 403.1 | 396.3 | 391.4 | 146.3 | 127.6 | 103.2 |
 10  | 24  | 523.0 | 514.5 | 496.4 | 14.23 | 13.36 | 20.66 | 757.0 | 748.9 | 734.2 | 881.4 | 864.0 | 489.9 |
 10  | 32  | 592.7 | 575.3 | 511.8 | 14.92 | 14.03 | 24.12 | 936.3 | 933.0 | 755.5 | 1237. | 1356. | 233.9 |

Improve CA* [1], regress **m [2] and Ca* does not benefit[3] as much from this optimization.

[1] This patch does not recompile on a shape guard failure, but use an inline cache to keep the same loads & stores if the share guard outermost guarded shape is common to all objects flowing in.

[2] The mostly case is regressed from ~213ms to ~370ms - ~755ms (*am) because when a shape guard is failing, we remember that we are seeing a polymorphic case, even if the use case is mostly-monomorphic.

[3] I would have expected to see on Ca* the same improvement as observed on CA*, as the Type Object should not interfere in the process.  I guess this issue is related to Bug 834614, where a monomorphic / monoparent case is converted to a polymorphic one.

Comment 7

[Nicolas B. Pierron [:nbp]]

I cannot reproduce any performance difference with the pdfjs.js patch.