Closed
Bug 1275292
Opened 8 years ago
Closed 6 years ago
Delete ParseNodeAllocator::freelist, freeTree(), and prepareNodeForMutation()
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: jorendorff, Assigned: jorendorff)
Details
Attachments
(1 file)
38.54 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
The ParseNode freelist is unlikely to save much memory, and each freeTree() call site is hand-rolled weirdness. It's plausible that freeing nodes could be causing bugs. It's also kind of a lot of code.
Assignee | ||
Comment 1•8 years ago
|
||
There are two comments that seem to say that freeing the node is something we do to "aggressively" "verify" that there are no errors in nearby code. 8-|
Assignee | ||
Comment 2•8 years ago
|
||
MozReview-Commit-ID: GInhmTbuXlw
Attachment #8756566 -
Flags: review?(shu)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Comment 3•8 years ago
|
||
With my Uptime hat on, this change looks like a wonderful simplification :)
With my MemShrink hat on, I wonder about this:
> The ParseNode freelist is unlikely to save much memory
Do you have measurements?
Comment 4•8 years ago
|
||
BTW, this change was prompted by bug 127488.
Assignee | ||
Comment 5•8 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #3) > > The ParseNode freelist is unlikely to save much memory > > Do you have measurements? Nope! Here's why I think this. 1. Parse nodes are transient. After compilation, "free" or not, we reclaim the entire arena in which they're allocated. So all this matters for, at most, a brief time during bytecode compilation. 2. The total number of parse nodes is proportional to script length, which is constrained by how much web sites want to send over the network and how much time they want the browser to spend in parsing. So the spikes are, eh, not huge. I could measure this. 3. The only place where we free nodes in order to save memory is after each top-level statement in a script. The pattern (before this patch) is: parse one statement; emit bytecode; free the AST; repeat. Two trends in web dev make this increasingly irrelevant. (a) For the past 5-10 years there's been a trend toward putting the entire script in a single IIFE anyway, for sanity's sake, and then there's only one top-level statement in the script -- I think this might be true of *all* webpack-generated script, for instance. We could measure this effect. (b) Eventually the Web will use modules. When we parse modules, we don't free anything. (Happily, modules themselves may serve as smaller units of work for the bytecode compiler...) Which is not to say I refuse to measure, but I need to think about it.
Assignee | ||
Comment 6•8 years ago
|
||
I forgot to add in #3: (c) We now have a **much** better mechanism to reduce the high-water-mark of memory used for ParseNodes per parse than freeTree, thanks to bhackett: lazy parsing of functions (we never free anything while parsing such functions --- basically can't). (thinking) Yeah - freeTree is useless. It really needs to go.
Comment 7•8 years ago
|
||
Some good news: about:memory has a measurement called "js-main-runtime-temporary-peak" which measures almost perfectly the peak use of parse nodes. I implemented it back in the day when we sometimes had 100s of MiBs of them. In more recent time I haven't seen it go above 2 or 3 MiB. So you could check that easily just to be safe.
Comment 8•8 years ago
|
||
Comment on attachment 8756566 [details] [diff] [review] Delete ParseNodeAllocator::freeTree() and friends Review of attachment 8756566 [details] [diff] [review]: ----------------------------------------------------------------- This is wonderful. It would take like a 100% memory use regression to justify not landing this. ::: js/src/frontend/FoldConstants.cpp @@ +783,5 @@ > // condition -- `a || true || expr` or |b && false && expr| -- then > // trailing nodes will never be evaluated. Truncate the list after > // the known-truthiness node, as it's the overall result. > if ((t == Truthy) == isOrNode) { > ParseNode* afterNext; Is afterNext dead now? Please remove if so. @@ +784,5 @@ > // trailing nodes will never be evaluated. Truncate the list after > // the known-truthiness node, as it's the overall result. > if ((t == Truthy) == isOrNode) { > ParseNode* afterNext; > + for (ParseNode* next = (*elem)->pn_next; next; next = next->pn_next) { Nit: no need for { } anymore.
Attachment #8756566 -
Flags: review?(shu) → review+
Assignee | ||
Comment 9•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef8ee9057091219e563e215d729c626f7a6e948b
Assignee | ||
Comment 10•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd76679b6765313bc7643f9a942cfd7a305424f7
Assignee | ||
Comment 11•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5dab7ec9caa3ed779954ec6d59e96c836df71fb8
Assignee | ||
Comment 12•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b512c7a263b259b844aefd723301fb7643e1bfd2 Bug 1275292 - Delete ParseNodeAllocator::freeTree() and friends. r=shu.
Comment 13•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b512c7a263b2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•