1925128 - School Edsby website hangs

Status: NEW

(Core :: JavaScript Engine, defect, P2)

Description

[Neha Kochar [:neha]]

​​My son's school uses an edsby website for parent-teacher comms and it used to work smoothly before and recently (maybe since last 2 months) it hangs. I captured a profile with it: https://share.firefox.dev/3A1CihU . Hopefully this helps.
The website https://rotherglen.edsby.com requires login so I'm happy to collect more info if you can direct me to collect whatever you might need to fix it.

Steps to Reproduce:

1. Login to https://rotherglen.edsby.com
2. On the left menu, click on the child name to get info for the child's class.

Expected Results:
Class teacher posts including pictures are shown in chronological order.

Actual Results:
None of the class posts load, the content area is empty.
The content finally loads after 2 minutes.

Profile URL:
https://share.firefox.dev/3A1CihU

Comment 1

[Neha Kochar [:neha]]

Doing the same in Chrome, it isn't immediate but the content on the page loads in 1-2 seconds.

Comment 2

[Markus Stange [:mstange]]

The time is spent doing nursery collections while parsing JSON. https://share.firefox.dev/3zP5VDe

It would be interesting to find out which JSON is being parsed, so that we can turn it into an anonymized testcase.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

The JS code is accessible here: https://rotherglen.edsby.com/static/compiled/dc8774a59c9f69e949d2d7a88904af1e60e052af92eaedfe5f85c1bc71bab5db/core/engine.min.js

Comment 4

[Jeff Muizelaar [:jrmuizel]]

I believe the JSON parsing is coming from here:

        _clone_internal: function(e) {
            if (e instanceof A) {
                let t = "c";
                return b.deserializeClass(e.serialize(t), t);
            }
            try {
                let t = Math.random(), a = JSON.stringify(e, function(e, a) {
                    return "object" == typeof this[e] && this[e] instanceof A ? this[e].serialize(t) : a;
                });
                return JSON.parse(a, function(e, a) {
                    return "object" == typeof a ? b.deserializeClass(a, t) : a;
                });
            } catch (e) {}
        }

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Neha, can you check if it's a regression using mozregression?

Comment 6

[Jan de Mooij [:jandem]]

This is likely a regression from the json-parse-with-source feature we implemented (bug 1913085 or an earlier bug).

Because it's calling JSON.parse with a reviver function, we maintain "parse record" objects that basically mirror the parsed JSON objects but store source information for it. We're fully tracing these maps on each nursery GC it looks like.

Comment 7

[Jan de Mooij [:jandem]]

For the JS shell micro-benchmark below:

--setpref=experimental.json_parse_with_source=false: 450 ms
--setpref=experimental.json_parse_with_source=true : 13442 ms

Bryan, as a first step I think we should flip the pref, definitely on beta, because it will give us more time to fix this regression on Nightly.

function f() {
    var o = [];
    for (var i = 0; i < 100000; i++) {
        o.push({x: "foo".repeat(10) + i, y: "bar".repeat(10) + i});
    }
    var json = JSON.stringify(o);
    var res;
    var t = new Date;
    for (var i = 0; i < 5; i++) {
        res = JSON.parse(json, (key, value) => value);
    }
    print(new Date - t);
    return res;
}
f();

Comment 8

[Markus Stange [:mstange]]

(unsetting needinfo for Neha, I don't think we need the regression range anymore)

Comment 9

[Bryan Thrall [:bthrall]]

We expected the json-parse-with-source feature to make calling JSON.parse with a reviver function more expensive, but this is worse than I expected.

Most of the cost appears to be from allocating JSStrings for the "source", but about a third of it comes from just allocating the context parameter to the reviver.

I'll look into reducing the cost from those two allocations.

Comment 10

[Jan de Mooij [:jandem]]

I think the bigger problem is the GC fully tracing the parse record object roots. You can see this in the flame graph tab of the profiles in comment 0 and comment 2, and in a profile for the micro-benchmark in comment 7.

The tracing gets slow for large object graphs, because there's more and more stuff to trace through.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1913085

Comment 12

[Jon Coppeard (:jonco)]

I think the bigger problem is the GC fully tracing the parse record object roots.

Yes, the problem is that there's a large data structure on the stack which is getting traced every time we do a minor GC. One fix for this would be to use JSObjects to hold the ParseRecordObject tree. These would become tenured and since nursery tracing stops when it reaches a tenured object this would greatly reduce the amount of stuff traced.

Another improvement we could make is to pass gcHeap as the third parameter to NewStringCopy in JSONReviveHandler<CharT>::finishPrimitiveParseRecord. Half the minor GCs in the profile come from that site. This would stop allocating source strings in the nursery when the output tree becomes too large to fit in the nursery.

Comment 13

[Jon Coppeard (:jonco)]

(In reply to Jon Coppeard (:jonco) from comment #12)
I also wondered about the possibility of calling the reviver functions as we parse the JSON tree so avoiding the need to create this data structure in the first place. I didn't check the spec too closely so maybe that doesn't work.

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1913085