Closed Bug 1275292 Opened 8 years ago Closed 6 years ago

Delete ParseNodeAllocator::freelist, freeTree(), and prepareNodeForMutation()

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: jorendorff, Assigned: jorendorff)

Details

Attachments

(1 file)

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.
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-|
MozReview-Commit-ID: GInhmTbuXlw
Attachment #8756566 - Flags: review?(shu)
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
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?
BTW, this change was prompted by bug 127488.
(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.
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.
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 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+
https://hg.mozilla.org/mozilla-central/rev/b512c7a263b2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.