1901316 - 5.71 - 4.09% pdfpaint issue15292.pdf / pdfpaint pr8808.pdf + 2 more (Linux, Windows) regression on Mon June 3 2024

Status: RESOLVED WONTFIX

(Firefox :: PDF Viewer, defect)

Description

[Andrej (:aglavic)]

Perfherder has detected a talos performance regression from push c76a7635ae4d1ec9db25fa15241d479decc7e76b. As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)
6%	pdfpaint issue15292.pdf	linux1804-64-shippable-qr	e10s fission stylo webrender-sw	431.52 -> 456.14
5%	pdfpaint issue15292.pdf	linux1804-64-shippable-qr	e10s fission stylo webrender	433.50 -> 456.81
4%	pdfpaint pr8808.pdf	windows10-64-shippable-qr	e10s fission stylo webrender	265.84 -> 276.93
4%	pdfpaint pr8808.pdf	windows10-64-shippable-qr	e10s fission stylo webrender-sw	266.92 -> 277.83

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
41%	pdfpaint bug1077808.pdf	linux1804-64-shippable-qr	e10s fission stylo webrender	3,025.06 -> 1,773.47
41%	pdfpaint bug1077808.pdf	linux1804-64-shippable-qr	e10s fission stylo webrender-sw	3,000.46 -> 1,767.62
32%	pdfpaint bug1077808.pdf	macosx1015-64-shippable-qr	e10s fission stylo webrender	2,105.15 -> 1,428.35
32%	pdfpaint bug1077808.pdf	windows10-64-shippable-qr	e10s fission stylo webrender	1,926.91 -> 1,313.27
32%	pdfpaint bug1077808.pdf	windows10-64-shippable-qr	e10s fission stylo webrender-sw	1,863.03 -> 1,271.70
...	...	...	...	...
2%	pdfpaint issue13794.pdf	macosx1015-64-shippable-qr	e10s fission stylo webrender-sw	1,027.86 -> 1,006.15

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests. Please follow our guide to handling regression bugs and let us know your plans within 3 business days, or the patch(es) may be backed out in accordance with our regression policy.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 613

For more information on performance sheriffing please see our FAQ.

Comment 1

[Andrej (:aglavic)]

This regressions most likely from the benchmark update, in which case this can be closed quickly as it is a baseline change rather than a regression

Comment 2

[Marco Castelluccio [:marco]]

This is likely a change due to https://github.com/mozilla/pdf.js/pull/18167. It might be slower in some cases because we have to fallback when DecompressionStream fails, but these cases are very rare because they are badly formatted PDFs. In the most common cases, it results in huge improvements.

Comment 3

[Calixte Denizet (:calixte)]

The regressions with pr8808.pdf and issue15292.pdf were expected because some compressed streams contained some extra data and consequently the DecompressionStream is throwing a Unexpected input after the end of stream and we're falling back on our pure js decompression stuff.
I know this exception is thrown because of the specs but in our case at least it's pretty useless.
:saschanaz, would it be possible to not throw in the pdf.js case only ? Or do you know if there's an easy trick to guess the position of the end of the compressed data (in our case the data is an array, see https://github.com/mozilla/pdf.js/blob/c770e94e362feddc5f64e683f8014a3dc0711f4f/src/core/flate_stream.js#L157-L197 for details) ?

Comment 4

[Kagami Rosylight [:saschanaz] (they/them) (OOO until 2024-10-06)]

Hmm, unfortunately there's no good way currently: https://github.com/whatwg/compression/issues/39

Is it the case that the PDF format does not have a header field to tell the exact chunk size? We could draft our own ChromeOnly API and propose that to the spec, if this is urgent.

Comment 5

[Calixte Denizet (:calixte)]

In the pdf we just have the length of the stream.
In pr8808.pdf there is one extra byte 0x0A when in issue15292.pdf there are 2 extra bytes 0x0, 0x0.
These bytes shouldn't be there and it's because of the pdf producer which was buggy.
That said this issue should be pretty rare.
From a spec pov, maybe it'd be better to just provides the decompressed bytes, then throw an exception and let the devs decide what they want to do: either keep the decompressed data or get rid of them (it's just my two cents).

Comment 6

[Kagami Rosylight [:saschanaz] (they/them) (OOO until 2024-10-06)]

Hmm yeah, could be nice to throw a special exception object that includes the decompressed (and unread) bytes so far instead of a simple TypeError: https://searchfox.org/mozilla-central/rev/aa9d148d5be3e7b606448f0b8da6e9f4fa43112f/dom/base/DecompressionStream.cpp#164

Comment 7

[Calixte Denizet (:calixte)]

Yep, I didn't think about the leftover bytes because in the pdf case we don't care about them, but in general yes I think it'd be nice to have something like that.