glimmerjs / glimmer-vm

MIT License
1.13k stars 190 forks source link

"Opcodes" Are Megamorphic #356

Closed chadhietala closed 7 years ago

chadhietala commented 7 years ago

I think it may be time to move away from objects as "opcodes" (hmmm sounds weird) and instead just use an array of strings. Because the "opcodes" are megamorphic the append on the LinkedList never JITs making it very slow. Using an Array and slice is likely better here.

/cc @krisselden

stefanpenner commented 7 years ago

@chadhietala could we consider TS constants as opcodes? That way they resolve to simple small numbers.

chadhietala commented 7 years ago

Yea I believe so. I think to have more specific types of opcodes

compile(vm) {
  vm.stack().push(this.tagName);
  vm.append(GET_TAG, CREATE_ELEMENT);
}
...
'get-tag'() {
  return this.tag = this.frame.unshift();
},
'create-element'() {
  this.document.createElement(this.tag, this.element);
}
stefanpenner commented 7 years ago

@chadhietala an interesting thing, that we may not already do. Is during startup, glimmer to run through a template that hits all the opcodes, so that all reasonable paths are discovered before the the code gets HOT and a optimized compile occurs, that way the IC's may be primed with most/all reasonable inputs. And later encountering of new op-codes wont de-opt.

krisselden commented 7 years ago

@stefanpenner even if enough of the paths are covered to make it JIT as a generic field load it is still much slower than a loop over an array. Another issue with the linked-list is it creates depth from the GC roots. I think a linked list of arrays of opcodes would be ok if they need to be spliced or inserted.

stefanpenner commented 7 years ago

@krisselden what I proposed doesn't prevent changes to how the op codes are stored/used/implemented.

chadhietala commented 7 years ago

Closing this now as we've taken the first steps to move away from the old architecture. The updating program is still a LL of opcode objects but the append side uses a byte array.