Open Bug 1830195 Opened 3 years ago Updated 11 months ago

Investigate optimization of constant variables

Categories

(Core :: JavaScript Engine, task, P2)

Product:
Core
Component:
JavaScript Engine
Type:
task

Tracking

()

People

(Reporter: iain, Unassigned)

References

(Blocks 1 open bug,
URL
)

Details

(Whiteboard: [sp3])

If a variable is never mutated after it is initialized, then we could treat it as a constant and optimize it differently. For example, V8 generates faster code than we do for:

let obj = { x: 3 };
function foo() { return obj.x; }

The shapeguard for obj.x is elided because the parser has determined that obj is immutable.

Turning off this optimization in V8 makes a significant difference to performance. According to Jeff, this patch:

diff --git a/src/ast/variables.h b/src/ast/variables.h
index 590390b3160..55960ed186c 100644
--- a/src/ast/variables.h
+++ b/src/ast/variables.h
@@ -84,7 +84,8 @@ class Variable final : public ZoneObject {
   bool is_used() { return IsUsedField::decode(bit_field_); }
   void set_is_used() { bit_field_ = IsUsedField::update(bit_field_, true); }
   MaybeAssignedFlag maybe_assigned() const {
-    return MaybeAssignedFlagField::decode(bit_field_);
+    //return MaybeAssignedFlagField::decode(bit_field_);
+    return MaybeAssignedFlag::kMaybeAssigned;
   }
   void clear_maybe_assigned() {
     bit_field_ = MaybeAssignedFlagField::update(bit_field_, kNotAssigned);

... gives the following results:

Preact/Total                            : 12.57±0.15   12.49±0.16 -0.33..0.17
  The 97.5% confidence interval for the difference is (-1.13%, -0.12%)
--
Backbone/Total                          : 46.23±0.48   46.18±0.49 -0.68..0.59
  The 97.5% confidence interval for the difference is (-0.56%, 0.34%)
--
React/Total                             : 61.9±0.63    60.56±1.41 -2.92..0.25
  The 97.5% confidence interval for the difference is (-2.76%, -1.22%)
--
React-redux/Total                       : 79.58±0.71   77.09±1.15 -3.86..-1.11
  The 97.5% confidence interval for the difference is (-3.57%, -2.55%)
--
Svelte/Total                            : 6.58±0.07    6.56±0.08 -0.14..0.1
  The 97.5% confidence interval for the difference is (-0.80%, 0.18%)
--
Angular/Total                           : 46.6±3.31    44.2±3.26 -6.99..2.17
  The 97.5% confidence interval for the difference is (-8.01%, -2.17%)

We should look into this and see if we can use a similar technique.

Clarifying slightly, the actual case is more like function moduleWrapper() { let moduleGlobalVar = { ... }; function foo() { return moduleGlobalVar.x; } return { foo }; } ), where we know that moduleGlobalVar can't escape and therefore cannot be mutated.

Whiteboard: [sp3]

I'm trying to make heads or tails of the -actual- optimization that V8 does based on that flag, and I'm starting to think there was a misread or miscommunication here (or perhaps this flag has multiple consumers that I'm missing).

What was originally discussed was the idea that in function moduleWrapper() { let moduleGlobalVar = { ... }; function foo() { return moduleGlobalVar.x; } return { foo }; } ), since the load of x is coming from an object that cannot escape and is totally initialized by the time foo is returned, the property access of x doesn't need to be shape guarded.

What I'm actually seeing is a bit different. Tracing the kNotAssigned flag, I see (the only?) consumer in the form of this selection of BytecodeArrayBuilder::kImmutableSlot. This flag helps choose the bytecode generated for a context load, using either LdaImmutableCurrentContextSlot or LdaImmutableContextSlot to do this load.

Looking at their implementations for this bytecode, we can see the baseline compiler doesn't really consume the information about immutability, simply punting to the regular mutable load. Only Maglev seems to pay attention to this. These calls all end up in this helper function

The actual optimization is here, where it takes advantage of a mode wherein "the generated code can rely on the function context to be a constant (known at compile-time). This opens new optimization opportunities, but prevents code sharing between different function contexts.".

Recasting this into SpiderMonkey lexicon, I think effectively what Maglev is doing is taking the actual environment object associated with a function and baking the environment object directly into the native code, avoiding an environment chain walk which would otherwise be required. There's a number of qualifiers and catches in the code around this optimization, suggesting to me that this will not be a quick-easy win.

I am going to keep looking at this; in general, if we had a better way to flag references as being to an environment chain object whose state we could make strong statements about, then I can imagine us being able to do interesting optimization.

Oh: the other thing to mention; the test case for this behaviour suggests that it's not the property access that's improved here, but rather simply the access to the captured binding.

ie, in our running example, it's the load of moduleGlobalVar that is improved here, not the access to property x.

It looks like there is a flag to disable maglev function context specialization.

Jeff, can you check how much of the performance delta you measured earlier is covered by flipping that flag?

Flags: needinfo?(jmuizelaar)

Ok, so it's still limited to their optimizing tiers and after the environment is created right? (ie, we'd not see any change in performance by disabling the MaybeAssigned analysis if we'd only run with Ignition+Sparkplug correct?)

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #6)

Ok, so it's still limited to their optimizing tiers and after the environment is created right? (ie, we'd not see any change in performance by disabling the MaybeAssigned analysis if we'd only run with Ignition+Sparkplug correct?)

That matches my understanding.

So: From the parser side, I think this is comparatively possible, and I see how to do it mostly (patch newAssignmentNode and a few other places to recursively call into a routine which will poke data into the UsedNameTracker (further complicating that poor component); how to communicate from the UsedNameTracker to the BytecodeEmitter needs a bit more thought, but doesn't seem insurmountable).

Keeping with our optimization architecture, the next step for this would then to emit a different bytecode than GetAliasedVar which has IC support. That IC would then memoize the destination environment from the first load (We'd therefore always use the first environment with the value, not necessarily the environment that actually matches the current call -- OK so long as our immutability analysis is complete.

My read on this is that it's a cool optimization we could get around to, but the requirement for perfect information in implementation means that it'll be a couple of weeks of work and maybe not the ideal thing for us to tackle now.

So I circled back and took a look at this. Wrote up my thoughts in this google doc here

One barrier is Syntax parsing; our syntax parser throws away information we would need to accurately mark a binding as immutable. We could conceivably decide this is OK, and simply mark all bindings as 'written' should we do a syntax parse, though it does feel weird.

I think in the short term, I'm going to let this stew and pursue some other avenues.

This comment in particular seems like a good description of their approach:

It seems that the best way to do this is to combine watchpointing the fact that a scope is instantiated once and watchpointing the fact that the variable won't be assigned again.
Basically, we start out by not watching closure variables. But we do set a watchpoint on whether create_activation was called once or more than once.
If in the DFG we encounter a load from a scoped variable and:

  • We know that this scope has only been created once, and
  • We know that the variable either has no watchpoint yet or has a still-valid watchpoint set, then:
  • We can create a watchpoint set for the variable (if necessary) and register watchpoints on both the created-once scope watchpoint set and the variable watchpoint set.

Then we just need to figure out how best to trigger a fireAll() when someone writes to the closure variable.

Converting to SM terminology, I think Scope -> Environment and Watchpoint -> Fuse.

This V8 design doc is mostly about the calling convention, but there's one interesting paragraph near the end:

Context-Specialized Optimized Code
Currently we can compile one JSFunction specialized to one particular defining context. As soon as we create a second closure, the latter one tiers up independently with a context-generic optimized code. Since the design in this document caches the dispatch handle on the JSFunctions this optimization can be kept. When creating the second closure we will allocate a fresh JSDispatchTable entry and use that one instead. The existing context-specialized function keeps its entry. If the context specialized version deoptimizes we merge it back by picking up the canonical entry from the feedback cell.
If I'm reading this properly, V8 compiles the first instance of a given function with the environment directly baked in to the native code. All subsequent instances share a separate environment-agnostic version. That might be a partial explanation for Matt's observation above:

Recasting this into SpiderMonkey lexicon, I think effectively what Maglev is doing is taking the actual environment object associated with a function and baking the environment object directly into the native code, avoiding an environment chain walk which would otherwise be required. There's a number of qualifiers and catches in the code around this optimization, suggesting to me that this will not be a quick-easy win.

Speculatively assuming that the first instance of a function is special doesn't seem like a terrible heuristic to me.

See Also: → 1914496
You need to log in before you can comment on or make changes to this bug.