google / closure-compiler

A JavaScript checker and optimizer.
https://developers.google.com/closure/compiler/
Apache License 2.0
7.41k stars 1.15k forks source link

performance problem with instanceof, goog.module, Firefox #2800

Open myphysicslab opened 6 years ago

myphysicslab commented 6 years ago

After changing my classes to use goog.module (instead of goog.provide and goog.scope) a performance test was 3 times slower under Firefox browser on MacOS (no change in Chrome and Safari browsers). The run-time of the test jumped from 0.92 seconds to 3.01 seconds.

I tracked the problem down to one particular change, and the culprit is an instanceof test in a method called intersectionPossible which is called 15171840 times during this particular test.

/** @override */
intersectionPossible(edge, swellage) {
  if (edge instanceof StraightEdge) {
    // Because straight/straight edges never interact (instead they only interact with
    // Vertexes) we can avoid some testing and get a performance gain by returning false
    // if the other edge is also a straight edge.
    return false;
  } else {
    return super.intersectionPossible(edge, swellage);
  }
};

By changing the test to look for a property instead, the performance is back to where it was before.

/** @override */
intersectionPossible(edge, swellage) {
  if (edge.isStraightEdge) {
    // Because straight/straight edges never interact (instead they only interact with
    // Vertexes) we can avoid some testing and get a performance gain by returning false
    // if the other edge is also a straight edge.
    return false;
  } else {
    return super.intersectionPossible(edge, swellage);
  }
};

/** This function is used to avoid an `instanceof` test, because those are slow
* on Firefox.
* @return {undefined}
*/
isStraightEdge() {};

I suppose this is a bug with Firefox, but I don't know enough about what changes goog.module makes to be able to report it to Firefox/Mozilla.

MatrixFrog commented 6 years ago

I assume you're doing the performance measurements on a compiled build, so in the module version it's something like

const {StraightEdge} = goog.require(...);
...
  if (edge instanceof StraightEdge) {

which if I remember correctly gets compiled to

if (edge instanceof otherModule.StraightEdge) {

so maybe it's taking longer to lookup the StraightEdge property on the module object?

myphysicslab commented 6 years ago

Here is the simple-compiled code using instanceof

mpl$lab$engine2D$StraightEdge.prototype.intersectionPossible=function(a,b){return a instanceof
mpl$lab$engine2D$StraightEdge?!1:mpl$lab$engine2D$AbstractEdge.prototype.intersectionPossible.call(this,a,b)};

Here is simple-compiled code using a boolean property

mpl$lab$engine2D$StraightEdge.prototype.intersectionPossible=function(a,b){return
a.isStraight?!1:mpl$lab$engine2D$AbstractEdge.prototype.intersectionPossible.call(this,a,b)};

I'm guessing that Firefox does more thorough checking on instanceof prototype chain stuff? In every case on this particular test it is true that edge instanceof StraightEdge, so it shouldn't need to search far up the prototype chain.

What's odd is that this performance change happened when switching to goog.module for the StraightEdge class (and it's super class and interface). Perhaps it has to do with the special closure that a module generates, which perhaps makes Firefox do some extra checks?

MatrixFrog commented 6 years ago

Here is the simple-compiled code using instanceof

That's the goog.module version right? What does the goog.provide version look like?

myphysicslab commented 6 years ago

Here is the simple-compiled goog.provide version (from commit 5e2f60)

myphysicslab.lab.engine2D.StraightEdge.prototype.intersectionPossible=function(a,b){
  return a instanceof myphysicslab.lab.engine2D.StraightEdge?
  !1:myphysicslab.lab.engine2D.StraightEdge.superClass_.intersectionPossible.call(this,a,b)
};

I would think this would be slower than the goog.module version because there are all those property lookups to resolve the namespace names.