The default bug view has changed. See this FAQ.

Factor frontend::EmitTree

RESOLVED FIXED in mozilla11

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cdleary, Assigned: cdleary)

Tracking

unspecified
mozilla11
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(35 attachments)

88.35 KB, patch
Details | Diff | Splinter Review
11.82 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
8.72 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
4.66 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
2.38 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
5.98 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
2.10 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
2.75 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
1.27 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
3.18 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
1.50 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
3.83 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
2.04 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
13.97 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
6.25 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
5.54 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
3.12 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
7.56 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
3.00 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
6.28 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
4.21 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
7.20 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
4.89 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
4.49 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
2.10 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
1.86 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
4.49 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
2.25 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
12.41 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
5.40 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
8.16 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
3.62 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
2.84 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
913 bytes, patch
Waldo
: review+
Details | Diff | Splinter Review
508 bytes, text/plain
Details
GCC makes 1KiB frames (bug 687951), which is bad for an inherently recursive function. It needs to be refactored.
It's now called frontend::EmitTree. Fancy!
Summary: Factor js_EmitTree → Factor frontend::EmitTree
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).
Created attachment 576257 [details] [diff] [review]
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".
Attachment #576257 - Flags: review?(jwalden+bmo)
Created attachment 576258 [details] [diff] [review]
0. Move function emit.
Created attachment 576261 [details] [diff] [review]
1. Factor function emit.
Attachment #576258 - Attachment description: Move function emit. → 0. Move function emit.
Created attachment 576262 [details] [diff] [review]
2. Move do emit.
Created attachment 576264 [details] [diff] [review]
3. Factor do emit.
Created attachment 576265 [details] [diff] [review]
4. Move while emit.
Created attachment 576266 [details] [diff] [review]
5. Factor while emit.
Created attachment 576267 [details] [diff] [review]
6. Move break emit.
Attachment #576266 - Attachment description: Factor while emit. → 5. Factor while emit.
Created attachment 576268 [details] [diff] [review]
7. Factor break emit.
Created attachment 576269 [details] [diff] [review]
8. Move continue emit.
Created attachment 576274 [details] [diff] [review]
9. Factor continue emit.
Created attachment 576276 [details] [diff] [review]
10. Move return emit.
Created attachment 576277 [details] [diff] [review]
11. Factor return emit.
Created attachment 576278 [details] [diff] [review]
12. Move left curly emit.
Created attachment 576279 [details] [diff] [review]
13. Factor left curly emit.
Created attachment 576281 [details] [diff] [review]
14. Move delete emit.
Created attachment 576282 [details] [diff] [review]
15. Factor delete emit.
Created attachment 576283 [details] [diff] [review]
16. Move call/new emit.
Created attachment 576284 [details] [diff] [review]
17. Factor call/new emit.
Created attachment 576285 [details] [diff] [review]
18. Move logical emit.
Created attachment 576286 [details] [diff] [review]
19. Factor logical emit.
Created attachment 576288 [details] [diff] [review]
20. Move inc/dec emit.
Created attachment 576289 [details] [diff] [review]
21. Factor inc/dec emit.
Created attachment 576291 [details] [diff] [review]
22. Move label emit.
Created attachment 576292 [details] [diff] [review]
23. Factor label emit.
Created attachment 576294 [details] [diff] [review]
24. Move/factor synthetic statements emit.
Created attachment 576295 [details] [diff] [review]
25. Move conditional expression emit.
Created attachment 576296 [details] [diff] [review]
26. Factor conditional expression emit.
Created attachment 576299 [details] [diff] [review]
27. Move object emit.
Attachment #576299 - Attachment description: 27. Factor object emit. → 27. Move object emit.
Created attachment 576300 [details] [diff] [review]
28. Factor object emit.
Created attachment 576301 [details] [diff] [review]
29. Move array emit.
Created attachment 576302 [details] [diff] [review]
30. Factor array emit.
Created attachment 576303 [details] [diff] [review]
31. Move unary emit.
Created attachment 576305 [details] [diff] [review]
32. Factor unary emit.
Created attachment 576307 [details]
Maximum nesting depth finder.

A short Python script to find the maximum function nesting depth before you hit the recursion limit.
Attachment #576258 - Flags: review+
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.
Attachment #576261 - Flags: review+
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.
Attachment #576262 - Flags: review+
Attachment #576264 - Flags: review+
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.
Attachment #576265 - Flags: review+
Attachment #576266 - Flags: review+
Attachment #576267 - Flags: review+
Attachment #576268 - Flags: review+
Attachment #576269 - Flags: review+
Attachment #576274 - Flags: review+
Attachment #576276 - Flags: review+
Attachment #576277 - Flags: review+
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.
Attachment #576278 - Flags: review?(jwalden+bmo)
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.
Attachment #576257 - Flags: review?(jwalden+bmo)
Attachment #576278 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #576279 - Flags: review+
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?
Attachment #576281 - Flags: review+
Attachment #576282 - Flags: review+
Attachment #576283 - Flags: review+
Attachment #576284 - Flags: review+
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.
Attachment #576285 - Flags: review+
Attachment #576286 - Flags: review+
Attachment #576288 - Flags: review+
Attachment #576289 - Flags: review+
Attachment #576291 - Flags: review+
Attachment #576292 - Flags: review+
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?  ;-) )
Attachment #576294 - Flags: review+
Attachment #576295 - Flags: review+
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.)
Attachment #576296 - Flags: review+
Attachment #576299 - Flags: review+
Attachment #576300 - Flags: review+
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))) {
Attachment #576301 - Flags: review+
Attachment #576302 - Flags: review+
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.
Attachment #576303 - Flags: review+
Attachment #576305 - Flags: review+
Blocks: 699520
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0e237b26fe2
Target Milestone: --- → mozilla11
https://hg.mozilla.org/integration/mozilla-inbound/rev/319c74e59fd4 (backout)
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=5d2f9f820042 (33 patches)
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.