Bug ID: JDK-8074484 More aggressive value discarding

Type: Enhancement
Component: core-libs
Sub-Component: jdk.nashorn
Affected Version: 9

Priority: P4
Status: Resolved
Resolution: Fixed
OS: generic
CPU: generic

Submitted: 2015-03-05
Updated: 2015-09-29
Resolved: 2015-03-11

JDK 8	JDK 9
8u60Fixed	9 b55Fixed

CodeGenerator code for loading a JoinPredecessorExpression (JPE) ignores whether it is the current discard. Combined with the fact that for loop's modify expressions are now JPEs this causes a typical postincrement in them to have unnecessary DUP/POP pairs, e.g. a typical for-loop "++i" will now be 

  iload 3
  iconst_1
  invokedynamic iadd(II)I
  dup
  istore 3
  pop

The issue is bit more widespread than that, though -- there's a lot of other expressions that could also better discard their subexpressions, e.g. COMMARIGHT. Even if the JPE is fixed, a for loop having "++i, ++j" will not do a discard on ++j as COMMARIGHT doesn't do loadAndDiscard on its RHS regardless of wether it itself should be discarded. Basically, a review of discard logic would be in order.

Being aggressive about removing (or not loading at all) discarded values from the stack has an additional benefit: smaller stack-saving prologue before optimistic operations. Optimistic operations need to ensure that all values on stack are a result of local loads. If some values aren't, we emit a prologue that does a sequence of {store|pop}/load to ensure this invariant. If discarded values aren't on stack, this prologue becomes considerably smaller. For extreme example, "i++" in test/script/basic/run-octane.js:210 "for (var i = 0; i < args.length; i++) {" looked like this before (I indented the cruft): aload 2 aload 2 invokedynamic dyn:getProp|getElem|getMethod:i(Object;)I dup_x1 istore 6 pop istore 5 iload 5 aload 2 iload 6 iconst_1 invokedynamic iadd(II)I invokedynamic dyn:setProp|setElem:i(Object;I)V pop goto LXXX Now it looks like this: aload 2 aload 2 invokedynamic dyn:getProp|getElem|getMethod:i(Object;)I iconst_1 invokedynamic iadd(II)I invokedynamic dyn:setProp|setElem:i(Object;I)V goto LXXX Regardless, this is likely also a performance regression in 8u40, because for-loop modify expressions ("i++") need to be wrapped in a JoinPredecessorExpression, so that threw off the discard logic. In pessimistic, it's less bad as we mostly get an extra dup and pop around i++. In optimistic, it's worse when "i" is not local but in scope - that's when we get the above large prologue before iadd(). Fortunately, this is rare and mostly happens on either program level or when the loop body contains a function expression that references the counter (or there's an eval around��). Making sure COMMARIGHT is better about discarding eliminates an untold number of DUP/POP pairs in earley-boyer, which is big on a programming style where every statement is a huge comma-separated expression list of assignments and function calls.

06-03-2015

At first I implemented this so that I was propagating discards into most ordinary side-effect free expressions (e.g. arithmetic, bit, comparison operators, etc.). Then I decided against it: if they are discarded, that's usually programmer sloppiness (e.g. assigning to a dead variable etc.). Yes, we could avoid evaluating discarded side-effect free expressions, but that'd mean the compiler would need to check the discard stack for every binary and unary node. The benefit of this is not entirely clear. On the other hand, expressions that are used for control-flow (comma, ternary, short-circuiting logical operators, and the synthetic join predecessor expression in for loop "modify" position) are often purposefully used for those side effects and discarded. I decided to balance the need for compiler to do more work and the potential payoff, and I only aggressively discard values for these expressions.

06-03-2015