651448 - Clear statement-lifetime artifacts after each top level statement

Status: NEW

(Core :: JavaScript Engine, defect)

Description

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

Spurred by discussion in bug 649576. As Jim pointed out to me, simply adding in decls.clear() to the bottom of the compileScript loop only caused two failures, both of which appear quite fixable:

1. regress-551763-2, which was due to an invalid optimization in the former code. Our shell actually binds a configurable property on the global object named "arguments" during setup (which is pretty evil). A test was performing |var arguments = 42; assert(delete arguments, false);| and cleverly-but-inappropriately deducing that arguments was a known non-configurable binding, at which point the constant folder replaced the unary deletion with a simple JSOP_FALSE. Even with the top level decls cleared out, there may be some more bugs along the lines of |var arguments = delete arguments;| -- I'll have to think through the state space a bit more in tests.

2. regress-452498-187, which happened due to a bug in the const semantics. Even *without* clearing decls, the following problem exists:

$ ./js -e 'const x = 42; for (x in {}); print("okay!")'
-e:1: SyntaxError: invalid for/in left-hand side:
-e:1: const x = 42; for (x in {}); print("okay!")
-e:1: ...................^
$ ./js -e 'const x = 42' -e 'for (x in {}); print("okay!")'
okay!

Separating out translation units for top level statements shouldn't have this kind of effect. Normally we communicate global binding information through the shapes on the global object.

The question that I have to ask in order to fix this correctly: does binding to a read-only global var (as observed at a compile time) trigger a syntax error? I could see a counter argument being "the first thing a script may want to do is delete a read-only but configurable global object property in order to make later top-level code valid".

Comment 1

[Jim Blandy :jimb]

You da man.

I don't think binding to a read-only variable is ever a SyntaxError (and all SyntaxErrors are early errors; see chapter 16). It's assigning to 'eval' or 'arguments' in strict mode code that's an error.

Comment 2

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

(In reply to comment #1)
> I don't think binding to a read-only variable is ever a SyntaxError

Okay, so the new question becomes why the latter example didn't raise a TypeError at runtime for binding the read-only global shape x.

Comment 3

[Jeff Walden [:Waldo]]

Assignment to non-writable, non-configurable bindings is only a TypeError in strict mode code.

Comment 4

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

Attachment: WIP: double barrel arenas.

Proof of concept that separates out the lifetimes and removes recycling, on top of my arena rewrite in bug 684039. Passes jit/js tests.

Nicer way of separating the lifetimes in the parser/codegen datastructures (to make sure we clear everything properly) is clearly desirable.

(I wanted to be able to test the benefit of a very large primary chunk in the arena rewrite bug, but it's a lame experiment without a properly cleared statement arena.)

Comment 5

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

Attachment: Test tweaks required.

regress-422269.js: conservative stack scanner breaks this fragile "leak" test.
regress-452498-187.js: const in eval scope used as a LHS really shouldn't be a syntax error -- that implies that the checks for const are syntactic instead of semantic. Should probably remove the associated code path as well.
regress-551763-2.js: frontend is confusing |delete arguments| with var-declared arguments when there's already a configurable binding on the global. We should file a separate bug to remove that invalid optimization (Jason may have already filed one).

Comment 6

[Nicholas Nethercote [inactive]]

This sounds very promising for reducing memory usage and/or churn.  Some stats (high-water mark for each pool?  total number of parsenodes created?) before and after would be great!

Comment 7

[Jim Blandy :jimb]

Comment on attachment 558899 [details] [diff] [review]
WIP: double barrel arenas.

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

Overall comment: huzzah! This is exactly what I expected.

::: js/src/jscntxt.h
@@ +191,5 @@
>  
>      /* Temporary arena pool used while compiling and decompiling. */
>      static const size_t TEMP_POOL_PRIMARY_CHUNK_SIZE = 1 << 12;
>      js::QuadArena               tempPool;
> +    js::QuadArena               boxPool;

Why does the compiler allocate from LifoAllocs in ThreadData? Couldn't the lifos be members of Compiler, so that their storage is owned by the compiler? I guess that would be a separate patch.

If that's not practical, it seems like having a LifoAllocScope as a member of the Compiler would release the storage naturally, as a replacement for tempPoolMark.

(BTW, I think LifoAlloc is a much better term than 'arena' or 'pool'. Says what it means, rather than alluding to it, but still terse.)

::: js/src/jsparse.cpp
@@ -507,5 @@
>  
>  } /* namespace js */
>  
>  /*
> - * Push the children of |pn| on |stack|. Return true if |pn| itself could be

Ahh, the smell of deleted horrible code in the morning...

@@ +838,5 @@
>  
>      inDirectivePrologue = true;
>      tokenStream.setOctalCharacterEscape(false);
>      for (;;) {
> +        QuadArenaScope qas(&JS_THREAD_DATA(cx)->tempPool);

I would want a comment above this. If one isn't alert to the amazing semantics of LifoAllocScope, then it'd be a surprise.

Comment 8

[Jim Blandy :jimb]

Does that feedback cover what you wanted me to look at?

Is it really the case that all the failures this change caused were due to bogus optimizations and bad tests? It's astonishing that we had such hairy code to preserve so little.

Comment 9

[Till Schneidereit [:till]]

cdleary won't work on this, anymore. Might still be MemShrink material, though.