BytecodeEmitter::finishTakingSrcNotes is quadratic, and can take multiple seconds on large JavaScript files.

RESOLVED FIXED in Firefox 66

Status

()

defect
RESOLVED FIXED
5 months ago
3 months ago

People

(Reporter: gleachkr, Assigned: iain)

Tracking

65 Branch
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

(Whiteboard: [qf:p1:pageload], [qa-66b-p2])

Attachments

(2 attachments)

Reporter

Description

5 months ago
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/11.0 Safari/605.1.15

Steps to reproduce:

go to https://carnap.io/about in a recent firefox (it appears to affect both the recent beta and older versions)


Actual results:

In Firefox, the site takes much longer to load than in either webkit or chrome, and it runs a cpu at 100% throughout the loading process. 

A perf log is available at: https://perfht.ml/2EYztPN


Expected results:

The site, including JS, should load in roughly 3 seconds, as it does in other browsers.
Whiteboard: [qf]
Thanks for opening this issue.

The Quantum Flow (qf) team will help us evaluate the priority and find specific items to get fixed in order to address this issue.
Flags: needinfo?(kvijayan)

Updated

4 months ago
Whiteboard: [qf] → [qf:p1:pageload]

Bas posted a profile for this: https://perfht.ml/2VGoa4T

We seem to be spending a massive amount of time in source-notes, vector::append. Like, that is 90% of the time being spent in JS in that profile. Iain can you take a look at this, given your recent interest in notes-related things?

Flags: needinfo?(kvijayan) → needinfo?(iireland)

It's Vector::insert, to prepend something to the vector. Worth debugging or instrumenting the code in finishTakingSrcNotes and addToSrcNoteDelta to see what it's doing exactly.

Summary: Firefox has very bad performance loading heavy javascript (40MB) on carnap.io. → BytecodeEmitter::finishTakingSrcNotes is quadratic, and can take multiple seconds on large JavaScript files.
Assignee

Comment 4

4 months ago

After looking at this yesterday, I'm pretty sure we've figured out what's going on. finishTakingSrcNotes tries to update the offset of the first main note to account for the length of the prologue. In addToSrcNoteDelta, we either adjust the delta of the note itself, or insert a new xdelta. But a single xdelta can only represent offsets up to 64. If the prologue is very large, then we will insert many xdeltas. Each of these insertions will add one byte to the very beginning of the main srcnote vector, which is also presumably very large.

Parts of this code are at least 15 years old. Presumably nobody expected such large JS functions. Ted already wanted to redesign srcnotes for memshrink/perf reasons; this is just another piece of evidence. In the short term, this specific issue is easy to fix. The next thing we do with srcnotes is copy both the prologue and the main section into a single buffer. There's no reason to prepend these xdeltas to the main section; we can get the same effect by appending them to the prologue.

Assignee: nobody → iireland
Flags: needinfo?(iireland)
Assignee

Comment 6

4 months ago

Comment #4 is technically incorrect: we don't currently copy the srcnotes for the prologue, because there aren't any. That said, the prologue does already have a srcnotes vector, so it's trivial to copy it if we are going to put xdeltas inside (and conceptually it makes sense that the xdeltas which encode "skip past these prologue bytecodes" are stored in the srcnotes for the prologue).

Running the big carnap script (https://carnap.io/static/ghcjs/allactions/out.js) in a shell, this cuts our time by about 90%, which should put us in line with other browsers.

Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Comment 7

4 months ago
Pushed by iireland@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b4bda4351a9c
Improve performance of BytecodeEmitter::finishTakingSrcNotes r=djvj

Comment 8

4 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Assignee

Comment 9

4 months ago

I just noticed that I forgot to delete the declaration of addToSrcNoteDelta from the header file when I removed its implementation. Reopening to fix that.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 11

4 months ago
Pushed by iireland@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/49bb9b849c4a
Remove dead declaration from BytecodeEmitter.h r=tcampbell
Assignee

Updated

4 months ago
Status: REOPENED → RESOLVED
Last Resolved: 4 months ago4 months ago
Resolution: --- → FIXED
Blocks: 1521787
Whiteboard: [qf:p1:pageload] → [qf:p1:pageload], [qa-66b-p2]
You need to log in before you can comment on or make changes to this bug.