1376954 - consider marking NodeList [ProbablyShortLivingWrapper]

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Ben Kelly [:bkelly, not reviewing]]

NodeList is live updated and therefore expensive to maintain.  Many scripts will do a query, check an attribute, and then throw it away, though.  It would be nice if we could GC these short lives NodeList objects faster.  I guess [ProbablyShortLivingWrapper] is the way to get these nursery collected.

Olli suggests there may be a problem using this with proxies, though.

Comment 1

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

Yeah, ProbablyShortLivingWrapper doesn't seem to do anything with proxies.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Does adding a check for it to CGDOMProxyJSClass.define and adding JSCLASS_SKIP_NURSERY_FINALIZE to "flags" as needed work?

As in, I'm pretty sure it works in terms of passing the JSClass flag to the JS engine.  The question is whether that gives us nursery allocation, for a proxy.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Also, we should either make [ProbablyShortLivingWrapper] work on non-wrappercached things or fail codegen if you try to use it there.

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK, I have patches for this, but they won't work correctly (as in, either not allocate the object in the nursery, or crash if they do) without the blocking JS engine bugs fixed as well.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Ensure that we don't try to nursery-allocate non-wrappercached DOM objects

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Support nursery allocation of DOM proxy objects

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Allow nursery allocation of DOM nodelists

Comment 8

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

Comment on attachment 8882088 [details] [diff] [review]
part 1.  Ensure that we don't try to nursery-allocate non-wrappercached DOM objects

ah, sorry, didn't recall that moving part yesterday.
Thanks for adding the comment.

Comment 9

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

Comment on attachment 8882089 [details] [diff] [review]
part 2.  Support nursery allocation of DOM proxy objects

oh, Proxies have canNurseryAllocate method.
Could you add some comment to CGDOMJSProxyHandler_canNurseryAllocate explaining that it is for overriding the default canNurseryAllocate method in Proxy object.
(or maybe that is somehow obvious from the context not visible in the patch?)

Comment 10

[(no longer active)]

Boris, if you profile running Speedometer with these patches applied, how much does MatchNameAttribute show up (please test on top of the fix for bug 1376936).  See bug 1376936 comment 8, it seems like that fix helped cut down the cost of that function by about half, and I'm hoping that this bug would hopefully cut down the rest of the time as well, but it's hard to say since it depends on when the nursery GC will happen.

I mostly want to know whether we need to keep bug 1376695 itself as a Speedometer dependency or not!  Thanks a lot.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Boris, if you profile running Speedometer with these patches applied

These patches have no effect until the blocking spidermonkey bugs are fixed; see comment 4...

Comment 12

[(no longer active)]

So we only need a fix for bug 1377055 right?  Since the other one has a patch...

Comment 13

[Boris Zbarsky [:bzbarsky]]

Right.  I need to get hold of jonco and talk through some things to fix that one.

That said, I'd also love instructions for what the current best-practice is for profiling Speedometer, and which parts of it I should profile per comment 10.

Comment 14

[Boris Zbarsky [:bzbarsky]]

OK, so here's what I see locally, when loading http://speedometer2.benj.me/InteractiveRunner.html?suite=jQuery-TodoMVC then starting profiler, clicking Run, stopping the profiler once the test is done:

1)  Without any local patches applied, I see about 4.6ms spent under MatchNameAttribute.
2)  With bug 1376936 applied, I also see about 4.6ms spent under MatchNameAttribute.
3)  With the patches for bug 1376936, bug 1377056, and bug 1377368 applied, I see 6.0ms under MatchNameAttribute.

(That said, as far as I can tell all these numbers are fairly noisy...)

If I'm not measuring the right testcase, please let me know.

Comment 15

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

yeah, I don't trust the ms numbers of individual runs.
Would need to write some microbenchmark and run it several times to get more reliable numbers.

Comment 16

[(no longer active)]

I suggest running the full test suite instead and profile the first few hundred or so test runs.  See the steps in bug 1376594 comment 6.  I seem to get fairly reliable numbers this way.

Comment 17

[Boris Zbarsky [:bzbarsky]]

OK, using those steps and the three situations from comment 14:

1) No patches: 14.6ms, see http://bit.ly/2txRdMT
2) With bug 1376936 applied: 1.7ms, see http://bit.ly/2txKwup
3) With the other patches applied (also including this bug, of course): No samples in MatchNameAttr at all, see http://bit.ly/2txWPXr

Make of that what you will, esp the fact that going from (1) to (2) I see a much bugger than 2x drop.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6dfa4b51c236
part 1.  Ensure that we don't try to nursery-allocate non-wrappercached DOM objects.  r=smaug
https://hg.mozilla.org/integration/autoland/rev/7acbc4152930
part 2.  Support nursery allocation of DOM proxy objects.  r=smaug,jonco
https://hg.mozilla.org/integration/autoland/rev/6acce622c977
part 3.  Allow nursery allocation of DOM nodelists.  r=smaug

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6dfa4b51c236
https://hg.mozilla.org/mozilla-central/rev/7acbc4152930
https://hg.mozilla.org/mozilla-central/rev/6acce622c977