consider marking NodeList [ProbablyShortLivingWrapper]

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: bkelly, Assigned: bzbarsky)

Tracking

(Depends on 1 bug, Blocks 2 bugs)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(3 attachments)

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.
Yeah, ProbablyShortLivingWrapper doesn't seem to do anything with proxies.
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.
Also, we should either make [ProbablyShortLivingWrapper] work on non-wrappercached things or fail codegen if you try to use it there.
Depends on: 1377055
Depends on: 1377056
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.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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.
Attachment #8882088 - Flags: review?(bugs) → review+
Attachment #8882090 - Flags: review?(bugs) → review+
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?)
Attachment #8882089 - Flags: review?(bugs) → review+
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.
Flags: needinfo?(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...
Flags: needinfo?(bzbarsky)
So we only need a fix for bug 1377055 right?  Since the other one has a patch...
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.
Attachment #8882089 - Flags: review?(jcoppeard) → review+
Depends on: 1377190
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.
Flags: needinfo?(ehsan)
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.
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.
Flags: needinfo?(ehsan)
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.
Depends on: 1377368
Pushed by bzbarsky@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/6dfa4b51c236
https://hg.mozilla.org/mozilla-central/rev/7acbc4152930
https://hg.mozilla.org/mozilla-central/rev/6acce622c977
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.