Closed
Bug 1183400
Opened 9 years ago
Closed 9 years ago
Perform constant-folding by examining node kind, not arity-then-kind with "syntactic context" information threaded through all folding
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: Waldo, Assigned: Waldo)
References
Details
Attachments
(34 files, 1 obsolete file)
Next up in the list. To be sure, some people would rather nuke this entire component from orbit than modify it, clean it up, etc. But there are components of this that are indisputably necessary -- converting obj["prop"] to obj.prop, folding together string additions, possibly others although less clearly. And these changes help out code that doesn't quite make it to Ion. So let's fix what's there and defer these sorts of bigger arguments to another day.
Assignee | ||
Comment 1•9 years ago
|
||
The current setup now is that we do a first round of folding sub-nodes based upon the node's arity, then we arbitrarily decide to stop there if we're folding in deletion context, then we do a second round of folding based upon the node's kind, then we try to fold constants into true/false inside conditionals. The desired setup is a single switch based on node arity. However, getting from here to there is a bit tortuous. Where possible, I'm going to *try* to add cases to this new kind-switch that *completely* handle the entirety of folding. Sometimes this has to inline the if-deleting/if-condition bits, exactly as written. Other times, I end the case with a |goto afterFolding;| that skips the if-delete case and the secondary kind-switch, jumping straight to the condition code. Things must get worse, sometimes, before they can get better. But never fear! By the end of the patchwork here, Fold() contains one switch on kind, every case (or group of them) performs all steps of folding, and the only fallthrough from the switch is to a crash. And as a bonus, SyntacticContext dies. (It really only ever was a hack for our being unable to have folding of conditions, and of deletions, be more careful how they folded those sub-components.) So, buckle in. I'll try to keep the patch-stream to a handful at a time so it's not overwhelming.
Attachment #8633185 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 2•9 years ago
|
||
Some of these are further folded by the after-folding code, so I jump there and skip the by-kind folding that targets none of these. And as already-simplified nullary nodes, they're not going to change if touched in deletion contexxt, so who cares about that test, either. :-)
Attachment #8633187 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 3•9 years ago
|
||
There are two variants of |typeof|: that on a name, where if the name doesn't exist it's as if it evaluated to |undefined|; and that on an expression, fully evaluated. Because of the PNK_TYPEOF{NAME,EXPR} split, folding an expression into a name still preserves a PNK_TYPEOFEXPR node, and the emitter treats *EXPR nodes as always having expression semantics. So we can fold away without worries, for expressions. Names, on the other hand, don't fold into anything at all: they just stay names. So for PNK_TYPEOFNAME, just return true straightaway, after a few structural assertions. The truthiness of typeof isn't known by Boolish, so no need to proceed to afterFolding code. (If our IR split effects from result, we *could* know the result of typeof was truthy, but we don't do that, so eh.)
Attachment #8633191 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 4•9 years ago
|
||
Again, an effects/result split would make this Condition bit better, but oh well. And to reiterate, by the end of this bug's patches this Condition bit disappears entirely, so there's no reason to common up this condition-folding code in a method or anything. And the early-return for delete context doesn't matter because nothing treats PNK_VOID specially (even as part of a catch-all) in the arity-switch.
Attachment #8633195 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 5•9 years ago
|
||
The also-morph bit of this is a little fruity. However, if we do this, SyntacticContext::Delete becomes unnecessary, and the fruity is limited to exactly one place. Seems good enough. (Actual removal waits awhile, tho.) You'll also observe that this is pretty amenable to removing PNK_DELETESUPER* whenever you get to that, I hope. :-)
Attachment #8633203 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 6•9 years ago
|
||
Boolean inversion seems rather different from the arithmetic operators, to me -- and needs a different SyntacticContext at present -- so I split it away from the other mathematical unary operations. The unary arithmetic folding code stops when a number's been reached, without folding (in condition context) to true/false. There's no reason to go further as the old code did -- Boolish handles numbers just fine. In this way kind-wise operation makes for doing less stuff.
Attachment #8633209 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 7•9 years ago
|
||
When folding ++/--, I want to assert that the target remains valid through the folding. That requires having a parser instance around. Lots of drear here, nothing very interesting happening. Judging from my last skim now, I leave as an exercise for the reader determining when the options parameter became unused here. :-\
Attachment #8633216 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 8•9 years ago
|
||
These are basically just unary, except for the extra assertion. I'll take the extra assertion over sharing code, myself.
Attachment #8633220 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 9•9 years ago
|
||
Okay, this one's the first complicated patch. I've basically just ported the existing algorithm; I've simply given variables new names and added a bunch more comments explaining what's being done. I am not going to explain what the code does -- if the comments don't make it clear, then I probably should add more comments, or otherwise make changes here. Tell me what's unclear, if necessary, and I can fix it. I also eliminated the trailing if-Condition constant folding. If the list doesn't fold to one element, then Boolish would return Unknown, so no reason to bother. But if it *did* return Truthy/Falsy, then all we're left with is a node that's the overall result of the expression. And if that's in another condition, it'll *again* get folded down as needed, or be consumed as the result of the operation. The only difference I see is that |if ("foo")| might have been if-true before, and now it'd be if-"foo". This is most easily fixed at the end of the patches, at which point there's a FoldCondition method that would be the most ideal place to add a final replace-even-truthy-stuff-with-true step. If it really even matters, generally. :-)
Attachment #8633228 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8633230 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 11•9 years ago
|
||
I'll cut it off there and wait for a few of these to be reviewed before posting more. And actually, I think I was mistaken about the if-"foo" thing above. With the patches posted applied, I get: js> dis(); if ("foo") print(); loc op ----- -- main: 00000: getgname "dis" 00005: gimplicitthis "dis" 00010: call 0 00013: setrval 00014: getgname "print" 00019: gimplicitthis "print" 00024: call 0 00027: setrval 00028: retrval Source notes: ofs line pc delta desc args ---- ---- ----- ------ -------- ------ 0: 1 14 [ 14] xdelta 1: 1 14 [ 0] colspan 6 3: 1 14 [ 0] colspan 12 5: 1 28 [ 14] xdelta 6: 1 28 [ 0] colspan 8 That's certainly eliminating the test entirely, so I think there's no even micro-regression in folding of conditions in that bit.
Updated•9 years ago
|
Attachment #8633185 -
Flags: review?(efaustbmo) → review+
Comment 12•9 years ago
|
||
Comment on attachment 8633187 [details] [diff] [review] Constant-fold nullary nodes Review of attachment 8633187 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/FoldConstants.cpp @@ +627,5 @@ > case PNK_BREAK: > case PNK_CONTINUE: > + case PNK_TEMPLATE_STRING: > + case PNK_THIS: > + case PNK_GENERATOR: Is there a reason for this reordering? I'm not seeing why this is better than before
Attachment #8633187 -
Flags: review?(efaustbmo) → review+
Comment 13•9 years ago
|
||
Comment on attachment 8633191 [details] [diff] [review] Fold typeof nodes by examining their kind Review of attachment 8633191 [details] [diff] [review]: ----------------------------------------------------------------- Makes sense.
Attachment #8633191 -
Flags: review?(efaustbmo) → review+
Comment 14•9 years ago
|
||
Comment on attachment 8633195 [details] [diff] [review] Fold |void <expr>| expressions by kind Review of attachment 8633195 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/FoldConstants.cpp @@ +652,5 @@ > + ParseNode*& expr = node->pn_kid; > + if (!Fold(cx, &expr, handler, options, inGenexpLambda, SyntacticContext::Other)) > + return false; > + > + if (sc == SyntacticContext::Condition) { Good lord this is ugly. I would have thought we could fold this to undefined, and then undefined would be falsey anyways when we evaluated the condition. Am I missing some reason why we need false explicitly?
Attachment #8633195 -
Flags: review?(efaustbmo) → review+
Comment 15•9 years ago
|
||
Comment on attachment 8633203 [details] [diff] [review] Fold delete nodes by kind Review of attachment 8633203 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. ::: js/src/frontend/FoldConstants.cpp @@ +745,5 @@ > + if (!Fold(cx, &expr, handler, options, inGenexpLambda, SyntacticContext::Other)) > + return false; > + > + MOZ_ASSERT(expr->isKind(oldKind), > + "kind should have remained invariant under folding"); Cute. Nice assertion. @@ +800,5 @@ > > + case PNK_DELETENAME: { > + MOZ_ASSERT(pn->isArity(PN_UNARY)); > + MOZ_ASSERT(pn->pn_kid->isKind(PNK_NAME)); > + return true; Makes sense.
Attachment #8633203 -
Flags: review?(efaustbmo) → review+
Assignee | ||
Comment 16•9 years ago
|
||
Oh stab me with a fork, bug 1172895 did violence to all this. :-\ It's almost half-tempting to back out that change and just redo it myself atop all these patches, but for now I'm trying to rebase through it.
Comment 17•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3af9f3102f78 https://hg.mozilla.org/integration/mozilla-inbound/rev/b6f44f2906e2 https://hg.mozilla.org/integration/mozilla-inbound/rev/b9ddc2f99b1f https://hg.mozilla.org/integration/mozilla-inbound/rev/1b44dc3291f3 https://hg.mozilla.org/integration/mozilla-inbound/rev/673bfaaf157a
Assignee | ||
Comment 18•9 years ago
|
||
Rebasing seems to be be doable, if quite painful, but the end result might as well be me rewriting the relevant parts of that patch. Oh well. I may go back and change PNK_IF into a proper list node when this is all done, as a better fix for this. Whole bunch more patches to come. (In reply to Eric Faust [:efaust] from comment #12) > Is there a reason for this reordering? I'm not seeing why this is better > than before Version control hack to ensure the full list of nullary nodes was in the patch, for reviewing. Cue gps saying "but MozReview solves this for you". (In reply to Eric Faust [:efaust] from comment #14) > Good lord this is ugly. I would have thought we could fold this to > undefined, and then undefined would be falsey anyways when we evaluated the > condition. Am I missing some reason why we need false explicitly? That's basically right, but I'm just preserving the existing code. Ultimately I do simplify it that way once SyntacticContext dies.
Keywords: leave-open
Assignee | ||
Comment 19•9 years ago
|
||
I'm not sure how important it actually is that ?:?:?:... iteratively fold without consuming linear stack space, but the current code does so. (Probably because it's impossible to disentangle if/?: and for no other reason.) Keep doing that. This plus the next patch to disentangle if-nodes mean all of the changes in bug 1172895 have been extricated into the narrowest by-kind operations, so subsequent patching can be simple and understandable again. I recommend reviewing this patch and the next one before doing any of the rest.
Attachment #8636360 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 20•9 years ago
|
||
It *almost* feels to me like the isDefn() checks in here are supposed to guard against a function expression morphing into a function statement, but I haven't made it work to get a bug if I disable the don't-replace-if-defn logic. Not worth caring enough about, IMO, to figure it out 100%.
Attachment #8636361 -
Flags: review?(efaustbmo)
Comment 21•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3af9f3102f78 https://hg.mozilla.org/mozilla-central/rev/b6f44f2906e2 https://hg.mozilla.org/mozilla-central/rev/b9ddc2f99b1f https://hg.mozilla.org/mozilla-central/rev/1b44dc3291f3 https://hg.mozilla.org/mozilla-central/rev/673bfaaf157a
Assignee | ||
Comment 22•9 years ago
|
||
Attachment #8636896 -
Flags: review?(efaustbmo)
Assignee | ||
Updated•9 years ago
|
Attachment #8636361 -
Attachment is obsolete: true
Attachment #8636361 -
Flags: review?(efaustbmo)
Comment 23•9 years ago
|
||
Comment on attachment 8633209 [details] [diff] [review] Fold !/~/+/- by kind Review of attachment 8633209 [details] [diff] [review]: ----------------------------------------------------------------- Nice refactoring of the logic to shorten it. ::: js/src/frontend/FoldConstants.cpp @@ +779,5 @@ > + > + handler.prepareNodeForMutation(node); > + node->setKind(newval ? PNK_TRUE : PNK_FALSE); > + node->setArity(PN_NULLARY); > + node->setOp(newval ? JSOP_TRUE : JSOP_FALSE); As an aside, remind me why we are using the JSOp field of PNK_* nodes that represent emitting constants?
Attachment #8633209 -
Flags: review?(efaustbmo) → review+
Comment 24•9 years ago
|
||
Comment on attachment 8633216 [details] [diff] [review] Pass a Parser reference to Fold, not separate handler/options Review of attachment 8633216 [details] [diff] [review]: ----------------------------------------------------------------- APPROVED. ::: js/src/frontend/Parser.h @@ +389,5 @@ > /* State specific to the kind of parse being performed. */ > ParseHandler handler; > > + void prepareNodeForMutation(Node node) { handler.prepareNodeForMutation(node); } > + void freeTree(Node node) { handler.freeTree(node); } This is fine. parser.handler.foo() would have been fine as well.
Attachment #8633216 -
Flags: review?(efaustbmo) → review+
Comment 25•9 years ago
|
||
Comment on attachment 8633220 [details] [diff] [review] Fold increment/decrement operations by kind Review of attachment 8633220 [details] [diff] [review]: ----------------------------------------------------------------- Yup.
Attachment #8633220 -
Flags: review?(efaustbmo) → review+
Comment 26•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d146b7cb26a https://hg.mozilla.org/integration/mozilla-inbound/rev/5e702a894535 https://hg.mozilla.org/integration/mozilla-inbound/rev/21787f9ff39d
Assignee | ||
Comment 27•9 years ago
|
||
Attachment #8639658 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 28•9 years ago
|
||
This treats most binary operators using one method, but it separates out PNK_POW because it's simpler to handle separately. The next patch gets rid of FoldBinaryNumeric and eliminates about 35 lines of code, but it's separate for readability.
Attachment #8639662 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 29•9 years ago
|
||
Every reader of this file will be glad to see the clarification of the node-munging code into its caller, I think. :-)
Attachment #8639663 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 30•9 years ago
|
||
If the renaming of pn1/pn2/pn3 here doesn't make you cry sweet tears of joy, I don't know what will.
Attachment #8639665 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 31•9 years ago
|
||
I grant that this is complicated, but the logic is well-commented, and I think it tracks worlds more cleanly than the old logic did, with its weird mess of statefulness that I still half-believe to be buggy. And I'll call it for now with this latest batch of reviews. More to come once a handful of these have been reviewed.
Attachment #8639670 -
Flags: review?(efaustbmo)
Comment 32•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6d146b7cb26a https://hg.mozilla.org/mozilla-central/rev/5e702a894535 https://hg.mozilla.org/mozilla-central/rev/21787f9ff39d
Comment 33•9 years ago
|
||
Comment on attachment 8633228 [details] [diff] [review] Fold and/or expressions by kind Review of attachment 8633228 [details] [diff] [review]: ----------------------------------------------------------------- Great added comments.
Attachment #8633228 -
Flags: review?(efaustbmo) → review+
Comment 34•9 years ago
|
||
Comment on attachment 8633230 [details] [diff] [review] Fold function nodes by kind Review of attachment 8633230 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/FoldConstants.cpp @@ +937,5 @@ > + return true; > + > + // Note: pn_body is null for lazily-parsed functions. > + if (ParseNode*& functionBody = node->pn_body) { > + if (!Fold(cx, &functionBody, parser, node->pn_funbox->inGenexpLambda, This is cute. That type is a little much, but I kinda like it.
Attachment #8633230 -
Flags: review?(efaustbmo) → review+
Comment 35•9 years ago
|
||
Comment on attachment 8636360 [details] [diff] [review] Fold ?: expressions by kind Review of attachment 8636360 [details] [diff] [review]: ----------------------------------------------------------------- First documented comment I've seen codifying the fear I feel all the time when removing code. ;) ::: js/src/frontend/FoldConstants.cpp @@ +774,5 @@ > + if (!Fold(cx, &expr, handler, options, inGenexpLambda, SyntacticContext::Condition)) > + return false; > + > + ParseNode*& ifTruthy = node->pn_kid2; > + if (!Fold(cx, &ifTruthy, handler, options, inGenexpLambda, SyntacticContext::Other)) I'll just leave a remark here that the patch goes to great length to keep C?T: C2?T2:F chains from consuming linear stack space. It's also possible that we could write C ? C2 ? T : F : F2. This will get recursively folded. If we care, we can file a followup. @@ +811,5 @@ > + // a definition. > + // > + // The rationale for this pre-existing restriction is unclear; if you > + // discover it, please document it! This lack of explanation is > + // tolerated only because failing to optimize *should* always be okay. "_/o\_" Can we add a comment involving the word "speculatively" that describes the function nonsense we discussed IRL?
Attachment #8636360 -
Flags: review?(efaustbmo) → review+
Comment 36•9 years ago
|
||
Comment on attachment 8636896 [details] [diff] [review] Fold |if| nodes by kind, with a one-character =/== typo-fix and a targeted test Review of attachment 8636896 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/FoldConstants.cpp @@ +873,5 @@ > + > + // Eliminate the consequent or alternative if the condition has > + // constant truthiness. Don't eliminate if we have an |if (0)| in > + // trailing position in a generator expression, as this is a special > + // form we can't fold away. No way that should be PNK_IF, but OK. Greener grass over there. @@ +878,5 @@ > + Truthiness t = Boolish(expr); > + if (t == Unknown || inGenexpLambda) > + continue; > + > + // Careful! If we have |if (0) ...| with no |else|, the discarded Perhaps this comment belongs below with the |if (discarded)|? @@ +895,5 @@ > + if (discarded) { > + // A declaration that hoists outside the arm of the |if| prevents > + // the |if| from being folded away. > + bool containsHoistedDecls; > + if (!ContainsHoistedDeclaration(cx, discarded, &containsHoistedDecls)) This makes so much more sense to see than isDefn before. I am tempted to go back and request that we try to remove that, as I strongly suspect a RCU cargo-cult failure, in which we made a cargo culted copy of the old code without realizing that we non longer needed that piece, then updated the old code, but not the copy. @@ +916,5 @@ > + } else { > + // As with PNK_CONDITIONAL, replace only if the replacement isn't a > + // definition. As there, the rationale for this restriction is > + // unclear and undocumented: tolerated only because a failure to > + // optimize *should* be safe. Idgi... @@ +925,5 @@ > + // replacement requires folding) or clear it (if |alternative| > + // is dead code) as needed. > + if (nextNode) > + nextNode = (*nextNode == replacement) ? nodePtr : nullptr; > + ReplaceNode(nodePtr, replacement); we can factor this more, if we want, right? ReplaceConditionalNode or something? Probably not worth it, but will avoid some potentially fragile duplication
Attachment #8636896 -
Flags: review?(efaustbmo) → review+
Comment 37•9 years ago
|
||
Comment on attachment 8639658 [details] [diff] [review] Fold various simple unary cases by kind Review of attachment 8639658 [details] [diff] [review]: ----------------------------------------------------------------- APPROVED.
Attachment #8639658 -
Flags: review?(efaustbmo) → review+
Assignee | ||
Comment 38•9 years ago
|
||
Attachment #8640056 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 39•9 years ago
|
||
Attachment #8640057 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 40•9 years ago
|
||
Attachment #8640058 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 41•9 years ago
|
||
Attachment #8640060 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 42•9 years ago
|
||
Attachment #8640062 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 43•9 years ago
|
||
Attachment #8640074 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 44•9 years ago
|
||
Attachment #8640077 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 45•9 years ago
|
||
Attachment #8640078 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 46•9 years ago
|
||
Attachment #8640081 -
Flags: review?(efaustbmo)
Comment 47•9 years ago
|
||
Comment on attachment 8639662 [details] [diff] [review] Fold binary arithmetic operations by kind, not arity Review of attachment 8639662 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/FoldConstants.cpp @@ +1170,5 @@ > + > + // Repoint the list's tail pointer. > + node->pn_tail = listp; > + > + // Now fold all leading numeric terms together into a single number. please add a note that we can't fold trailing ones because of possible coercion to double.
Attachment #8639662 -
Flags: review?(efaustbmo) → review+
Comment 48•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/58129dccd5d8 https://hg.mozilla.org/integration/mozilla-inbound/rev/7dbbc282b2e7 https://hg.mozilla.org/integration/mozilla-inbound/rev/5b94aec178f1 https://hg.mozilla.org/integration/mozilla-inbound/rev/9eb62a986338 https://hg.mozilla.org/integration/mozilla-inbound/rev/c80e97aaf3b7 https://hg.mozilla.org/integration/mozilla-inbound/rev/d8e044ff8372
Comment 50•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/58129dccd5d8 https://hg.mozilla.org/mozilla-central/rev/7dbbc282b2e7 https://hg.mozilla.org/mozilla-central/rev/5b94aec178f1 https://hg.mozilla.org/mozilla-central/rev/9eb62a986338 https://hg.mozilla.org/mozilla-central/rev/c80e97aaf3b7 https://hg.mozilla.org/mozilla-central/rev/d8e044ff8372 https://hg.mozilla.org/mozilla-central/rev/284e48036311
Comment 51•9 years ago
|
||
Comment on attachment 8636896 [details] [diff] [review] Fold |if| nodes by kind, with a one-character =/== typo-fix and a targeted test Review of attachment 8636896 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/FoldConstants.cpp @@ -841,5 @@ > - > - bool mightHaveHoistedDeclarations = true; > - > - if (false) { > - restart: Nice.
Comment 52•9 years ago
|
||
Comment on attachment 8640056 [details] [diff] [review] Fold various list nodes not given any special treatment Review of attachment 8640056 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/FoldConstants.cpp @@ +1634,5 @@ > + case PNK_ARGSBODY: > + case PNK_CALLSITEOBJ: > + case PNK_EXPORT_SPEC_LIST: > + case PNK_IMPORT_SPEC_LIST: > + return FoldList(cx, pn, parser, inGenexpLambda); The FoldList definition ended up in the addition patch.
Comment 53•9 years ago
|
||
Comment on attachment 8640081 [details] [diff] [review] Fold class nodes by kind Review of attachment 8640081 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/FoldConstants.cpp @@ +1632,5 @@ > +{ > + MOZ_ASSERT(node->isKind(PNK_CLASS)); > + MOZ_ASSERT(node->isArity(PN_TERNARY)); > + > + // The first child of a class is a pair of names referring to it, Indentation
Assignee | ||
Comment 54•9 years ago
|
||
Comment on attachment 8640056 [details] [diff] [review] Fold various list nodes not given any special treatment Review of attachment 8640056 [details] [diff] [review]: ----------------------------------------------------------------- Shifting a few of the easiest reviews over, for some load-balancing...
Attachment #8640056 -
Flags: review?(efaustbmo) → review?(shu)
Assignee | ||
Comment 55•9 years ago
|
||
Comment on attachment 8640074 [details] [diff] [review] Constant-fold yield/yield*/return by kind Review of attachment 8640074 [details] [diff] [review]: ----------------------------------------------------------------- All the structure stuff copies what's in ParseNode.cpp, basically.
Attachment #8640074 -
Flags: review?(efaustbmo) → review?(shu)
Assignee | ||
Comment 56•9 years ago
|
||
Comment on attachment 8640081 [details] [diff] [review] Fold class nodes by kind Review of attachment 8640081 [details] [diff] [review]: ----------------------------------------------------------------- .
Attachment #8640081 -
Flags: review?(efaustbmo) → review?(shu)
Assignee | ||
Updated•9 years ago
|
Attachment #8640078 -
Flags: review?(efaustbmo) → review?(shu)
Comment 57•9 years ago
|
||
Comment on attachment 8640056 [details] [diff] [review] Fold various list nodes not given any special treatment Review of attachment 8640056 [details] [diff] [review]: ----------------------------------------------------------------- Yeah sure why not. I'll assume FoldList does something obvious.
Attachment #8640056 -
Flags: review?(shu) → review+
Comment 58•9 years ago
|
||
Comment on attachment 8640074 [details] [diff] [review] Constant-fold yield/yield*/return by kind Review of attachment 8640074 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/FoldConstants.cpp @@ +1515,5 @@ > +{ > + MOZ_ASSERT(node->isKind(PNK_RETURN)); > + MOZ_ASSERT(node->isArity(PN_BINARY)); > + > + if (ParseNode*& expr = node->pn_left) { Nit: probably clearer to just use &node->pn_left directly below.
Attachment #8640074 -
Flags: review?(shu) → review+
Updated•9 years ago
|
Attachment #8640078 -
Flags: review?(shu) → review+
Updated•9 years ago
|
Attachment #8640081 -
Flags: review?(shu) → review+
Assignee | ||
Comment 59•9 years ago
|
||
Cherrypicked from way down the patch queue for sooner-inspection, given PNK_SUPERPROP and PNK_SUPERELEM are soon going away.
Attachment #8645199 -
Flags: review?(efaustbmo)
Comment 60•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6019d456196c https://hg.mozilla.org/integration/mozilla-inbound/rev/efd0dbc0cf17 https://hg.mozilla.org/integration/mozilla-inbound/rev/0ca4340aaf49 https://hg.mozilla.org/integration/mozilla-inbound/rev/c753a5b5cfb5
Comment 61•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6019d456196c https://hg.mozilla.org/mozilla-central/rev/efd0dbc0cf17 https://hg.mozilla.org/mozilla-central/rev/0ca4340aaf49 https://hg.mozilla.org/mozilla-central/rev/c753a5b5cfb5
Comment 62•9 years ago
|
||
Comment on attachment 8639663 [details] [diff] [review] Inline FoldBinaryNumeric into its sole caller and simplify code accordingly Review of attachment 8639663 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comment addressed. ::: js/src/frontend/FoldConstants.cpp @@ +1061,5 @@ > } > > +static double > +ComputeBinary(ParseNodeKind kind, double left, double right) > +{ Shouldn't there be PNK_POW support here? It looks like there was before. Looks like a rebase mistake? Honestly justified, given the review lag.
Attachment #8639663 -
Flags: review?(efaustbmo) → review+
Comment 63•9 years ago
|
||
(In reply to Eric Faust [:efaust] from comment #62) > Comment on attachment 8639663 [details] [diff] [review] > Inline FoldBinaryNumeric into its sole caller and simplify code accordingly > > Review of attachment 8639663 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me with comment addressed. > > ::: js/src/frontend/FoldConstants.cpp > @@ +1061,5 @@ > > } > > > > +static double > > +ComputeBinary(ParseNodeKind kind, double left, double right) > > +{ > > Shouldn't there be PNK_POW support here? It looks like there was before. > Looks like a rebase mistake? Honestly justified, given the review lag. So, I get that it's right associative, so it can't be in this spot, but I would expect to see handling it somewhere?
Comment 64•9 years ago
|
||
Comment on attachment 8639665 [details] [diff] [review] Fold element accesses by kind Review of attachment 8639665 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/FoldConstants.cpp @@ +1227,5 @@ > + if (!Fold(cx, &expr, parser, inGenexpLambda, SyntacticContext::Other)) > + return false; > + > + ParseNode*& key = node->pn_right; > + if (!Fold(cx, &key, parser, inGenexpLambda, SyntacticContext::Other)) This is great. I'm going to wait to rebase the super stuff through this, as it's much easier. @@ +1277,5 @@ > + dottedAccess->setInParens(node->isInParens()); > + ReplaceNode(nodePtr, dottedAccess); > + > + // If we've replaced |expr["prop"]| with |expr.prop|, we can now free the > + // |"prop"| and |expr["prop"]| nodes -- but not the |expr| node now a sub- the second half of this line reads strangely. Perhaps "but not the |expr| node, as it's now..."?
Attachment #8639665 -
Flags: review?(efaustbmo) → review+
Comment 65•9 years ago
|
||
Comment on attachment 8639670 [details] [diff] [review] Fold addition by kind Review of attachment 8639670 [details] [diff] [review]: ----------------------------------------------------------------- This is really nice. ::: js/src/frontend/FoldConstants.cpp @@ +1311,5 @@ > +} > + > +static bool > +FoldAdd(ExclusiveContext* cx, ParseNode** nodePtr, Parser<FullParseHandler>& parser, > + bool inGenexpLambda) I like this very much. It's very well laid out, and easy to follow, even for doing a complicated thing.
Attachment #8639670 -
Flags: review?(efaustbmo) → review+
Comment 66•9 years ago
|
||
Comment on attachment 8640057 [details] [diff] [review] Fold function calls and tagged templates by kind Review of attachment 8640057 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/FoldConstants.cpp @@ +1494,5 @@ > + // var prop = "global"; > + // var obj = { prop: "obj", f: function() { return this.prop; } }; > + // assertEq((true ? obj.f : null)(), "global"); > + // assertEq(obj.f(), "obj"); > + // assertEq((true ? obj.f : null)``, "global"); This behavior is insane, but not your fault.
Attachment #8640057 -
Flags: review?(efaustbmo) → review+
Updated•9 years ago
|
Attachment #8640058 -
Flags: review?(efaustbmo) → review+
Updated•9 years ago
|
Attachment #8640060 -
Flags: review?(efaustbmo) → review+
Comment 67•9 years ago
|
||
Comment on attachment 8640062 [details] [diff] [review] Constant-fold switch/default by kind Review of attachment 8640062 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/FoldConstants.cpp @@ +1701,5 @@ > Fold(cx, &pn->pn_right, parser, inGenexpLambda, SyntacticContext::Other); > > + case PNK_DEFAULT: > + MOZ_ASSERT(pn->isArity(PN_BINARY)); > + MOZ_ASSERT(!pn->pn_left); What a strange thing to see. Why not use a unary node, then?
Attachment #8640062 -
Flags: review?(efaustbmo) → review+
Comment 68•9 years ago
|
||
Comment on attachment 8640077 [details] [diff] [review] Fold for-in, for-of, and for(;;) loops by kind, as well as various miscellaneous things Review of attachment 8640077 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/FoldConstants.cpp @@ +1770,5 @@ > Fold(cx, &pn->pn_right, parser, inGenexpLambda, SyntacticContext::Other); > > + case PNK_CLASSNAMES: > + MOZ_ASSERT(pn->isArity(PN_BINARY)); > + if (ParseNode*& outerBinding = pn->pn_left) { prefer to use pn->as<ClassNames>().outerBinding(), or whatever, if we can. Probably not, though, since the types don't line up. We still have to decide if things are operating over the union, or the substructs. Personally, I favor the latter, but we'll see.
Attachment #8640077 -
Flags: review?(efaustbmo) → review+
Updated•9 years ago
|
Attachment #8645199 -
Flags: review?(efaustbmo) → review+
Assignee | ||
Comment 69•9 years ago
|
||
And now, finally, the payoffs start to come. \o/
Attachment #8647211 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 70•9 years ago
|
||
Attachment #8647213 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 71•9 years ago
|
||
Attachment #8647214 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 72•9 years ago
|
||
Attachment #8647215 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 73•9 years ago
|
||
Attachment #8647216 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 75•9 years ago
|
||
Attachment #8647221 -
Flags: review?(efaustbmo)
Comment 76•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a7b272d43fa https://hg.mozilla.org/integration/mozilla-inbound/rev/639605af62de https://hg.mozilla.org/integration/mozilla-inbound/rev/f98f3d69b25e https://hg.mozilla.org/integration/mozilla-inbound/rev/db2af69165d8 https://hg.mozilla.org/integration/mozilla-inbound/rev/6a29efd82d94 https://hg.mozilla.org/integration/mozilla-inbound/rev/f24a684e6972 https://hg.mozilla.org/integration/mozilla-inbound/rev/fa9cd3d88b51 https://hg.mozilla.org/integration/mozilla-inbound/rev/056fdf25e862 https://hg.mozilla.org/integration/mozilla-inbound/rev/8a50c71ddeda
Comment 77•9 years ago
|
||
Comment on attachment 8647211 [details] [diff] [review] Remove dead fold-by-arity code Review of attachment 8647211 [details] [diff] [review]: ----------------------------------------------------------------- +1
Attachment #8647211 -
Flags: review?(efaustbmo) → review+
Comment 78•9 years ago
|
||
Comment on attachment 8647213 [details] [diff] [review] Remove SyntacticContext::Delete, now addressed by modifying how delete nodes are folded Review of attachment 8647213 [details] [diff] [review]: ----------------------------------------------------------------- Yuuup
Attachment #8647213 -
Flags: review?(efaustbmo) → review+
Comment 79•9 years ago
|
||
Comment on attachment 8647214 [details] [diff] [review] Replace callee-based condition-constant folding with caller-specified condition-constant folding Review of attachment 8647214 [details] [diff] [review]: ----------------------------------------------------------------- I guess the removal of SyntacticContext::Condition happens in a different patch?
Attachment #8647214 -
Flags: review?(efaustbmo) → review+
Comment 80•9 years ago
|
||
Comment on attachment 8647215 [details] [diff] [review] Remove special |void| handling by making Boolish recognize |void| expressions as falsy, when they're obviously so Review of attachment 8647215 [details] [diff] [review]: ----------------------------------------------------------------- See comment below for a thought. I don't know how good a thought it is, but it's sure a thought. ::: js/src/frontend/FoldConstants.cpp @@ +486,5 @@ > case PNK_NUMBER: > return (pn->pn_dval != 0 && !IsNaN(pn->pn_dval)) ? Truthy : Falsy; > > case PNK_STRING: > + case PNK_TEMPLATE_STRING: This can live here, but it's unrelated to this patch, right? @@ +518,5 @@ > + pn->isKind(PNK_NULL) || > + pn->isKind(PNK_FUNCTION) || > + pn->isKind(PNK_GENEXP)) > + { > + return Falsy; I don't really care that much, but I feel like this could also be case PNK_VOID: if (Boolish(pn->pn_kid) == Unknown) return Unknown; return Falsy; (You could still iterate down the void chain to avoid bad recursion) Probably this is too fragile? But we have a pretty good guarantee about the side effect free nature, if we can tell exactly what they evaluate to, I would think.
Attachment #8647215 -
Flags: review?(efaustbmo) → review+
Comment 81•9 years ago
|
||
Comment on attachment 8647216 [details] [diff] [review] Remove SyntacticContext::Condition, now handled context-sensitively by callers specifically requesting condition-targeted folding Review of attachment 8647216 [details] [diff] [review]: ----------------------------------------------------------------- WFM
Attachment #8647216 -
Flags: review?(efaustbmo) → review+
Comment 82•9 years ago
|
||
Comment on attachment 8647220 [details] [diff] [review] Remove SyntacticContext completely Review of attachment 8647220 [details] [diff] [review]: ----------------------------------------------------------------- Well done.
Attachment #8647220 -
Flags: review?(efaustbmo) → review+
Comment 83•9 years ago
|
||
Comment on attachment 8647221 [details] [diff] [review] Common up some is-effectless testing Review of attachment 8647221 [details] [diff] [review]: ----------------------------------------------------------------- Nice.
Attachment #8647221 -
Flags: review?(efaustbmo) → review+
Comment 84•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a6a4af92732 https://hg.mozilla.org/integration/mozilla-inbound/rev/aa4933454523 https://hg.mozilla.org/integration/mozilla-inbound/rev/60862f5c7abf https://hg.mozilla.org/integration/mozilla-inbound/rev/aacd11c8f3ca https://hg.mozilla.org/integration/mozilla-inbound/rev/7d13f9aefa3e https://hg.mozilla.org/integration/mozilla-inbound/rev/ef403529cdd7 https://hg.mozilla.org/integration/mozilla-inbound/rev/5c3304cf2c03
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Comment 85•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2a7b272d43fa https://hg.mozilla.org/mozilla-central/rev/639605af62de https://hg.mozilla.org/mozilla-central/rev/f98f3d69b25e https://hg.mozilla.org/mozilla-central/rev/db2af69165d8 https://hg.mozilla.org/mozilla-central/rev/6a29efd82d94 https://hg.mozilla.org/mozilla-central/rev/f24a684e6972 https://hg.mozilla.org/mozilla-central/rev/fa9cd3d88b51 https://hg.mozilla.org/mozilla-central/rev/056fdf25e862 https://hg.mozilla.org/mozilla-central/rev/8a50c71ddeda https://hg.mozilla.org/mozilla-central/rev/9a6a4af92732 https://hg.mozilla.org/mozilla-central/rev/aa4933454523 https://hg.mozilla.org/mozilla-central/rev/60862f5c7abf https://hg.mozilla.org/mozilla-central/rev/aacd11c8f3ca https://hg.mozilla.org/mozilla-central/rev/7d13f9aefa3e https://hg.mozilla.org/mozilla-central/rev/ef403529cdd7 https://hg.mozilla.org/mozilla-central/rev/5c3304cf2c03
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•