704369 - Factor frontend::EmitTree

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Chris Leary [:cdleary] (not checking bugmail)]

GCC makes 1KiB frames (bug 687951), which is bad for an inherently recursive function. It needs to be refactored.

Comment 1

[Chris Leary [:cdleary] (not checking bugmail)]

It's now called frontend::EmitTree. Fancy!

Comment 2

[Chris Leary [:cdleary] (not checking bugmail)]

Okay, I have a patch stack that takes us from 164->630 maximum function nesting depth on 32b, and 73->280 on 64b. It's a 33 patch stack (of tiny patches).

Comment 3

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Folded.

This folds all the patches, which I will attach individually. They follow the pattern of "patch to move the code, then follow up patch to clean it up".

Comment 4

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 0. Move function emit.

Comment 5

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 1. Factor function emit.

Comment 6

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 2. Move do emit.

Comment 7

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 3. Factor do emit.

Comment 8

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 4. Move while emit.

Comment 9

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 5. Factor while emit.

Comment 10

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 6. Move break emit.

Comment 11

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 7. Factor break emit.

Comment 12

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 8. Move continue emit.

Comment 13

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 9. Factor continue emit.

Comment 14

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 10. Move return emit.

Comment 15

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 11. Factor return emit.

Comment 16

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 12. Move left curly emit.

Comment 17

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 13. Factor left curly emit.

Comment 18

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 14. Move delete emit.

Comment 19

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 15. Factor delete emit.

Comment 20

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 16. Move call/new emit.

Comment 21

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 17. Factor call/new emit.

Comment 22

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 18. Move logical emit.

Comment 23

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 19. Factor logical emit.

Comment 24

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 20. Move inc/dec emit.

Comment 25

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 21. Factor inc/dec emit.

Comment 26

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 22. Move label emit.

Comment 27

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 23. Factor label emit.

Comment 28

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 24. Move/factor synthetic statements emit.

Comment 29

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 25. Move conditional expression emit.

Comment 30

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 26. Factor conditional expression emit.

Comment 31

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 27. Move object emit.

Comment 32

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 28. Factor object emit.

Comment 33

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 29. Move array emit.

Comment 34

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 30. Factor array emit.

Comment 35

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 31. Move unary emit.

Comment 36

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 32. Factor unary emit.

Comment 37

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Maximum nesting depth finder.

A short Python script to find the maximum function nesting depth before you hit the recursion limit.

Comment 38

[Jeff Walden [:Waldo]]

Comment on attachment 576261 [details] [diff] [review]
1. Factor function emit.

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

::: js/src/jscntxtinlines.h
@@ +120,5 @@
>  
> +template <typename T>
> +class AutoPtr
> +{
> +  private:

Luke at least has been omitting a leading |private:| in classes since it's the default -- probably worth doing that for consistency.

Comment 39

[Jeff Walden [:Waldo]]

Comment on attachment 576262 [details] [diff] [review]
2. Move do emit.

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +5887,5 @@
> +    ptrdiff_t noteIndex = NewSrcNote(cx, bce, SRC_WHILE);
> +    if (noteIndex < 0 || Emit1(cx, bce, JSOP_NOP) < 0)
> +        return JS_FALSE;
> +
> +    ptrdiff_t noteIndex2 = NewSrcNote(cx, bce, SRC_TRACE);

Beware that Luke will be changing this underneath you right pronto.

Comment 40

[Jeff Walden [:Waldo]]

Comment on attachment 576265 [details] [diff] [review]
4. Move while emit.

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +5954,5 @@
> +     * test at the top. When ParseNode trees were added during the ES3
> +     * work (1998-9), the code generation scheme was not optimized, and
> +     * the decompiler continued to take advantage of the branch and jump
> +     * that bracketed the body. But given the SRC_WHILE note, it is easy
> +     * to support the more efficient scheme.

This last paragraph of comment is perhaps historically interesting, but it doesn't really do anything to illuminate this code.  It also makes the code *seem* more complex, because you feel you have to read it to truly understand the code.  I think you should remove this paragraph (although whether in this patch or the next doesn't matter).

@@ +5964,5 @@
> +        return JS_FALSE;
> +    ptrdiff_t jmp = EmitJump(cx, bce, JSOP_GOTO, 0);
> +    if (jmp < 0)
> +        return JS_FALSE;
> +    ptrdiff_t noteIndex2 = NewSrcNote(cx, bce, SRC_TRACE);

Luke is also changing this too.

Comment 41

[Jeff Walden [:Waldo]]

Comment on attachment 576278 [details] [diff] [review]
12. Move left curly emit.

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

Leaving off here for the day.

Comment 42

[Jeff Walden [:Waldo]]

Comment on attachment 576257 [details] [diff] [review]
Folded.

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

I'm going to use my own self-r? to keep track of my incremental review-state here, now.

Comment 43

[Jeff Walden [:Waldo]]

Comment on attachment 576279 [details] [diff] [review]
13. Factor left curly emit.

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +6207,5 @@
> +     * labeled expression statements.  The tc->topStmt->update test
> +     * catches the case where we are nesting in EmitTree for a labeled
> +     * compound statement.
> +     */
> +    if (!useful &&

This if, minus its first condition, could move into the if above.  That would help reduce the amount of code to comprehend in EmitStatement when debugging through it by having a bigger block to step past.

Comment 44

[Jeff Walden [:Waldo]]

Comment on attachment 576279 [details] [diff] [review]
13. Factor left curly emit.

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

The previous patch moved left curly emit and statement emit both; this one pretties up only statement emit.  Did you forget to pretty up left curly emit?

Comment 45

[Jeff Walden [:Waldo]]

Comment on attachment 576285 [details] [diff] [review]
18. Move logical emit.

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +6421,5 @@
>      return true;
>  }
>  
> +static bool
> +EmitLogical(JSContext *cx, BytecodeEmitter *bce, ParseNode *pn)

"logical" is not a very meaningful name to me.  But it *is* what the spec uses, sigh.

Comment 46

[Jeff Walden [:Waldo]]

Comment on attachment 576294 [details] [diff] [review]
24. Move/factor synthetic statements emit.

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

You should have split this up into a move patch and a factor patch.

(...I kid, I kid!  Gotta find *something* to complain about in a 32-patch series, right?  ;-) )

Comment 47

[Jeff Walden [:Waldo]]

Comment on attachment 576296 [details] [diff] [review]
26. Factor conditional expression emit.

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +6671,2 @@
>       * exception handling code generator, that samples bce->stackDepth for
>       * use at runtime (JSOP_SETSP), or in let expression and block code

Do something about this comment.  JSOP_SETSP doesn't exist now, and to be honest I don't remember it ever existing in the time I've been working on SpiderMonkey code.  (Which isn't to say it predates that, but I'd say it's likelier than not that it does.)

Comment 48

[Jeff Walden [:Waldo]]

Comment on attachment 576301 [details] [diff] [review]
29. Move array emit.

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +7324,2 @@
>  # if JS_HAS_GENERATORS
> +        if (pn->isKind(PNK_ARRAYCOMP)) {

Might be worth doing this to unify two identical blocks:

  if (pn->isKind(PNK_RB) || (JS_HAS_GENERATORS && pn->isKind(PNK_ARRAYCOMP))) {

Comment 49

[Jeff Walden [:Waldo]]

Comment on attachment 576303 [details] [diff] [review]
31. Move unary emit.

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +7107,5 @@
>          break;
>  
>        case PNK_PLUS:
>        case PNK_MINUS:
> +        if (pn->isArity(PN_UNARY)) {

I think this part will be somewhat different due to PNK_POS/PNK_NEG now.

Comment 50

[Chris Leary [:cdleary] (not checking bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0e237b26fe2

Comment 51

[Chris Leary [:cdleary] (not checking bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/319c74e59fd4 (backout)
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=5d2f9f820042 (33 patches)

Comment 52

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/319c74e59fd4
https://hg.mozilla.org/mozilla-central/rev/6bc8f2c740f7
https://hg.mozilla.org/mozilla-central/rev/c27aaef8236b
https://hg.mozilla.org/mozilla-central/rev/c57c39973cea
https://hg.mozilla.org/mozilla-central/rev/29150131be3a
https://hg.mozilla.org/mozilla-central/rev/4229d7555c2f
https://hg.mozilla.org/mozilla-central/rev/52f6d44759e9
https://hg.mozilla.org/mozilla-central/rev/89da5f464940
https://hg.mozilla.org/mozilla-central/rev/2a4220da28db
https://hg.mozilla.org/mozilla-central/rev/62ea2d21d2ec
https://hg.mozilla.org/mozilla-central/rev/91b832b0d1e1
https://hg.mozilla.org/mozilla-central/rev/c3c8b90818c2
https://hg.mozilla.org/mozilla-central/rev/204075ca2adb
https://hg.mozilla.org/mozilla-central/rev/dc64909d1e94
https://hg.mozilla.org/mozilla-central/rev/0bd7c556a400
https://hg.mozilla.org/mozilla-central/rev/e79c2ba7ccfb
https://hg.mozilla.org/mozilla-central/rev/21b69bd24616
https://hg.mozilla.org/mozilla-central/rev/d32af7870b33
https://hg.mozilla.org/mozilla-central/rev/e773fc43b804
https://hg.mozilla.org/mozilla-central/rev/ead00f811c0a
https://hg.mozilla.org/mozilla-central/rev/33ce3483a420
https://hg.mozilla.org/mozilla-central/rev/3fa67cb7fa08
https://hg.mozilla.org/mozilla-central/rev/e2d2a99e4931
https://hg.mozilla.org/mozilla-central/rev/5da59ae1cf74
https://hg.mozilla.org/mozilla-central/rev/c0d430f2753b
https://hg.mozilla.org/mozilla-central/rev/ddd7fd60d1fc
https://hg.mozilla.org/mozilla-central/rev/2cd6087dbca3
https://hg.mozilla.org/mozilla-central/rev/e5963f5906f7
https://hg.mozilla.org/mozilla-central/rev/b09bfe78aa77
https://hg.mozilla.org/mozilla-central/rev/1a53e9f2b691
https://hg.mozilla.org/mozilla-central/rev/db81d88f016b
https://hg.mozilla.org/mozilla-central/rev/9ddf74776f7f
https://hg.mozilla.org/mozilla-central/rev/7d0b47a8e142
https://hg.mozilla.org/mozilla-central/rev/5d2f9f820042