1461012 - Add CC optimization for PrecompiledScript

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement, P3)

Description

[Andrew McCreight [:mccr8]]

One of the reasons that we're hitting slow code in bug 1449033 is that the single PrecompiledScript has a black wrapper, but it does not mark its mScript field black. This feels a little riskier than the patches that just skip the expensive tracing so maybe this should only go on Nightly.

Comment 1

[mathieuu]

Does that mean that the regression introduced by rev 1b4d5be72031 reported by bug 1449033 won't be fixed for Firefox 60?

Comment 2

[Andrew McCreight [:mccr8]]

(In reply to mathieuu from comment #1)
> Does that mean that the regression introduced by rev 1b4d5be72031 reported
> by bug 1449033 won't be fixed for Firefox 60?

By "the patches that just skip the expensive tracing" I mean the patches in bug 1460636 and bug 1460385. They seem to fix the problem for me, though I haven't tried the addon in bug 1449033 yet, which seems to be hit by the problem more. My preference would be to backport those to 60 rather than whatever happens here.

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1461012: Add CC optimizations for PrecompiledScript objects.

Tracing the script members held alive by PrecompiledScript objects can be
expensive, especially for large scripts. It would be nice if we could avoid
tracing them when possible.

Most of the time, when PrecompiledScripts are alive, they're held alive by
black roots, and have black JS wrappers. In those cases, there's no chance of
them creating garbage cycles, and we can just mark their script children
black.

Review commit: https://reviewboard.mozilla.org/r/243542/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243542/

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

I'm not familiar with PrecompiledScript ownership model, but can we detect certainly alive such objects when mozilla::dom::TraceBlackJS is called, and just trace the scripts as black from the beginning?
If that is not the case, why? Could we change the ownership somehow so that mozilla::dom::TraceBlackJS could be used for tracing?

Comment 5

[Andrew McCreight [:mccr8]]

PrecompiledScript is owned by a weak set in JS, so I don't think we can do that. You'd have to add some weak reference from C++ for TraceBlackJS to use.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Or add a private slot to the wrapper pointing to the script so that when the wrapper is traced black, so will the script?

Comment 7

[Kris Maglione [:kmag]]

I suppose we could keep a linked list of them and trace them whenever they have a black wrapper, but I don't think that would have any benefits over just doing the same checks from CanSkip.

Comment 8

[Kris Maglione [:kmag]]

(In reply to Olli Pettay [:smaug] from comment #6)
> Or add a private slot to the wrapper pointing to the script so that when the
> wrapper is traced black, so will the script?

I suppose we could, but the CanSkip hook already does that, and also lets us skip the whole PrecompiledScript object, so there probably wouldn't be any real benefit.

Comment 9

[Andrew McCreight [:mccr8]]

The advantage is that having the GC mark the objects black from the start is more efficient than having the GC first mark the object grey, then going through the inefficient UnmarkGray code to mark them gray. (GC marking is much faster.) I'm not sure how much of a difference it makes in practice.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Well, the wrapper might not yet be black when mozilla::dom::TraceBlackJS is called.
But doing things in mozilla::dom::TraceBlackJS, when possible, is faster since then object is traced just once (black). No need for marking first gray and then later in CanSkip to black.
If the scripts can be huge and tracing them slow, this is something to think about.

Comment 11

[Kris Maglione [:kmag]]

OK. Well, there doesn't seem to be much measurable overhead from this approach with the RES add-on, but if it turns out to still be a problem, I suppose those might be options.

Comment 12

[Andrew McCreight [:mccr8]]

I probably won't get to this review until Monday, but it is probably a good idea anyways to have a Nightly or two with bug 1460636 but not this one to see if the other bug is sufficient.

Comment 13

[Andrew McCreight [:mccr8]]

Comment on attachment 8975192 [details]
Bug 1461012: Add CC optimizations for PrecompiledScript objects.

https://reviewboard.mozilla.org/r/243542/#review249736

Comment 14

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e701e816acbd5b1167fc2715d393b400edd8849
Bug 1461012: Add CC optimizations for PrecompiledScript objects. r=mccr8

Comment 15

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/4e701e816acb