(perf) avoid object.keys on hot path for performance - simplify

[vigneshshanmugam]

• I am getting rid of this logic since its not used for anything else other than expression type

• Doing this yields an 1-1.5 seconds improvement on average on a large 2 MB files.

Before

minify-simplify 10689.635
minify-simplify 10661.954

After

minify-simplify 8829.373
minify-simplify 9054.459

This is not a huge improvement but still helps when used on bigger bundles.

• I am also not sure why iterating over ~250 elements in babel types and creating symbols in global symbol registry is really slow. Not sure if symbol creation logic is slow on v8. /cc @mathiasbynens Can you help here?

Sample code

const minify = require("../packages/babel-minify");
const fs = require("fs");
const path = require("path");

const code = fs.readFileSync(
  path.join(__dirname, "huge-file.js"), // replace it with file > 2 MB
  "utf-8"
);

console.time("minify");
minify(code);
console.timeEnd("minify");

Got to know about the issue on profiling it using chrome devtools.

[mathiasbynens]

I’m on vacation and haven’t looked at this in detail, but I’ll look into whether we can do anything to speed up symbol creation in V8. cc @bmeurer

[boopathi]

I made it like this to solve a more general case of different types (Expression, LogicalExpression, etc.. - but LogicalExpression was recently removed leaving out expression as the only type used now.) in pattern matching - keeping only Expression seems wrong usage and this entire file helpers can be removed.

[vigneshshanmugam]

Yeah i totally understand that this was for general use case. But when i was playing around, I figured out that we are not using it for anything other than expression and symbol creation was bit slow in v8. So thought of keeping it open here so we can figure out whats the best.

[bmeurer]

To me this function isNodeOfType looks like the reason for the slow-down. The t["is" + Symbol.keyFor(typeForSymbol)] property access looks particularly bad, since it's highly megamorphic and always passes a newly concatenated string, which means it'll always use the slow-path via the C++ runtime.

[vigneshshanmugam]

@bmeurer Thanks for finding out the root cause.

[bmeurer]

Did you verify my theory? I just guessed based on staring at your diff 😁

[vigneshshanmugam]

My bad, I did check now and don't think that was the cause. It does have a minor increase though

Before

After (Without polymorphic state)

Symbol creation logic

const typeSymbols = t => {
  // don't recompute
  if (Object.keys(types).length < 1) {
    t.TYPES.forEach(type => {
      types[type] = Symbol.for(type);
    });
  }
  return types;
};

Still the problem of creating too many symbols stays the same as i stated before.

[bmeurer]

That's really odd. Can you run with --runtime-call-stats and search for SymbolFor in the output?

[vigneshshanmugam]

We invoke twice per minification process. But we compute it only once and return it from cache for successive invocations.

[bmeurer]

Can you post the first 10 items from --runtime-call-stats?

[vigneshshanmugam]

Sure.

        JS_Execution  22563.74ms  65.10%       780   0.01%
                  GC   5822.25ms  16.80%      6042   0.06%
    FunctionCallback   1567.68ms   4.52%      7327   0.07%
    KeyedGetProperty    742.17ms   2.14%   3932424  36.55%
RecompileSynchronous    729.72ms   2.11%      1414   0.01%
          ArrayConcat    401.79ms   1.16%    588038   5.47%
          ObjectKeys    389.82ms   1.12%     35271   0.33%
          StringSplit    379.40ms   1.09%   1751662  16.28%
    WeakCollectionSet    325.77ms   0.94%    523176   4.86%
          ArrayShift    136.54ms   0.39%    439691   4.09%
          ArraySlice    120.08ms   0.35%    278001   2.58%
          SetProperty    109.09ms   0.31%    272005   2.53%
ParseFunctionLiteral    104.88ms   0.30%      4990   0.05%

[vigneshshanmugam]

The only relevant thing I can think of from the list is Object.keys which we are using as part of typeSymbols.

[vigneshshanmugam]

After few confirmations, changing Object.keys with simple check is way faster. results here https://gist.github.com/vigneshshanmugam/c766550ecd02292dcdfbf0bf013b9d3d

[vigneshshanmugam]

Yields around 300-400ms on average - Results -> https://gist.github.com/vigneshshanmugam/c766550ecd02292dcdfbf0bf013b9d3d

[bmeurer]

Awesome finding! I totally didn't realize that Object.keys is on the hot path here, sorry.