Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
myphysicslab opened this issue Jan 29, 2018 · 4 comments
Open

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

myphysicslab opened this issue Jan 29, 2018 · 4 comments

Comments

@myphysicslab
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

Here is the simple-compiled code using instanceof

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

@myphysicslab
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants