1221660 - Remove ParseNode::pn_offset

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

It turns out this is only used by the emitter, as scratch space, while emitting switch statements. But every node has the field and... I guess it's always populated? Totally silly.

While fixing that, I accidentally created a CaseClause subclass of ParseNode and eliminated PNK_DEFAULT (since all code everywhere depends on PKN_DEFAULT nodes having exactly the same layout as PNK_CASE nodes, which strikes me as more precarious than just having a single kind).

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: Part 1: Stop populating ParseNode::pn_offset most of the time

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: Part 2: Move pn_offset into a branch of the ParseNode::pn_u union. Add a subclass of ParseNode for PNK_CASE nodes. Merge PNK_DEFAULT with PNK_CASE

Comment 3

[Jason Orendorff [:jorendorff]]

Removing pn_offset probably doesn't save any memory on 64-bit platforms, because it was a uint32_t field and the struct was very neatly bitpacked before. Still, it's good to see anything like that deleted...

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 8683220 [details] [diff] [review]
Part 1: Stop populating ParseNode::pn_offset most of the time

Review of attachment 8683220 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.cpp
@@ +3273,5 @@
>      for (ParseNode* caseNode = cases->pn_head; caseNode; caseNode = caseNode->pn_next) {
>          if (switchOp == JSOP_CONDSWITCH && !caseNode->isKind(PNK_DEFAULT))
>              setJumpOffsetAt(caseNode->pn_offset);
> +
> +        // If this is a TABLESWITCH, we'll need to know this case's offset

"emitted as a", maybe.

@@ +3274,5 @@
>          if (switchOp == JSOP_CONDSWITCH && !caseNode->isKind(PNK_DEFAULT))
>              setJumpOffsetAt(caseNode->pn_offset);
> +
> +        // If this is a TABLESWITCH, we'll need to know this case's offset
> +        // later, when emitting the table. Store it in the node's pn_offset

Remove the comma.

@@ +3276,5 @@
> +
> +        // If this is a TABLESWITCH, we'll need to know this case's offset
> +        // later, when emitting the table. Store it in the node's pn_offset
> +        // (giving the field a different meaning vs. how we used it on the
> +        // immediately preceding line of code).

lol

::: js/src/frontend/ParseNode.h
@@ +582,5 @@
>      static_assert(NumDefinitionFlagBits + NumBlockIdBits <= 32,
>                    "This is supposed to fit in a single uint32_t");
>  
>      TokenPos            pn_pos;         /* two 16-bit pairs here, for 64 bits */
> +    int32_t             pn_offset;      /* scratch space for the emitter's use */

lol

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8683226 [details] [diff] [review]
Part 2: Move pn_offset into a branch of the ParseNode::pn_u union. Add a subclass of ParseNode for PNK_CASE nodes. Merge PNK_DEFAULT with PNK_CASE

Review of attachment 8683226 [details] [diff] [review]:
-----------------------------------------------------------------

Hmm.  I said I thought it might be better keeping two kinds and *not* having them share code.  But this is sort of nice, I guess, so maybe this works well enough.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +3079,3 @@
>      if (caseCount == 0 ||
>          (caseCount == 1 &&
> +         (hasDefault = firstCase->isDefault())))

These last two lines can probably be one-lined for marginal readability improvement.

@@ +3101,5 @@
>                  continue;
>  
>              MOZ_ASSERT(switchOp == JSOP_TABLESWITCH);
>  
> +            ParseNode* caseValue = caseNode->caseExpression();

In a separate patch, rs=me, would be nice to rename to caseExpr here.

@@ +3125,5 @@
>                  high = i;
>  
> +            // Check for duplicates, which require a JSOP_CONDSWITCH.
> +            // We bias i by 65536 if it's negative, and hope that's a rare
> +            // case (because it requires a malloc'd bitmap).

64K ought to be enough for anyone.

@@ +3167,1 @@
>          switchSize = (size_t)(JUMP_OFFSET_LEN * (3 + tableLength));

Remove the parens to have a constructor-style cast rather than a C-style cast.

@@ +3186,5 @@
> +            ParseNode* caseValue = caseNode->caseExpression();
> +            if (caseValue) {
> +                if (!emitTree(caseValue))
> +                    return false;
> +            }

Could you add some blank lines in this loop, somewhere?  Anywhere?  :-)

::: js/src/frontend/NameFunctions.cpp
@@ +475,3 @@
>              MOZ_ASSERT(cur->isArity(PN_BINARY));
> +            if (cur->pn_left) {
> +                if (!resolve(cur->pn_left, prefix))

if (ParseNode* caseExpr = cur->pn_left) {
  if (!resolve(caseExpr, prefix))

Comment 6

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> Hmm.  I said I thought it might be better keeping two kinds and *not* having
> them share code.  But this is sort of nice, I guess, so maybe this works
> well enough.

You did -- I thought it was nice this way, and meant to follow up with you on IRC, but never did...

C++ is so awful at discriminated unions (relative to languages with pattern-matching, like ML and Rust) that a single type, with cases indicated by an enum here or a null pointer there, often wins on clarity over a class hierarchy. I don't love it, but that's the language we've got to work with... :-|

Comment 7

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe2d3a9ec23d2f6a6e192481c4f796ac3e4e66a7
Bug 1221660 - Part 1: Stop populating ParseNode::pn_offset most of the time. r=Waldo.

https://hg.mozilla.org/integration/mozilla-inbound/rev/859e6e58cdd21b059ed7992cf13a7c9082fb44ac
Bug 1221660 - Part 2: Move pn_offset into a branch of the ParseNode::pn_u union. Add a subclass of ParseNode for PNK_CASE nodes. Merge PNK_DEFAULT with PNK_CASE. r=Waldo.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fe2d3a9ec23d
https://hg.mozilla.org/mozilla-central/rev/859e6e58cdd2