Improve rendering performance

[mohd-akram]

Avoid unnecessary copies via Utils.extend in hot paths.

This leads to a 2x improvement in my testing.

See #1991.

[jaylinski]

Thanks for the improvement! LGTM, security tests are also passing.

[aleen42]

any progress?

[jaylinski]

I was hoping to get a review from @nknapp, since he introduced the Utils.extend()-code in commit 2078c72 to address a security issue.

I'm also not sure if the new code causes side-effects, since it mutates an object now (before it created a copy).

As I said, the tests are good, so it should be good to merge. I'll wait for a week for @nknapp to share his thoughts, otherwise I'll merge it.

[nknapp]

I don't think it is good practice to modify the options object as a side-effect.

I can see that "Util.extend" may be slow, since it checks for "hasOwnProperty" all the time, but I would at least try to create a shallow copy of the object.

Maybe

const extendedOptions = {
    ...options,
    hooks: this.hooks,
    protoAccessControl: this.protoAccessControl
}

Could you check how the performance behaves for that code?
Apart from that, since there is a test for "blockHelperMissing" and "helperMissing", I don't think there is a security risk here, although I feel to far off the topic to give a good estimate.

What you could also try is just calling Object.assign instead of "Util.extend".

This just occured to me, but "Object.assign" seems to do extactly the same thing as "Util.extend", probably faster, since it is built-in.

For backwards compatibily with historic browsers, maybe in "utils.js" do

export const extend = Object.assign || function extend(obj/* , ...source */) {
  for (let i = 1; i < arguments.length; i++) {
    for (let key in arguments[i]) {
      if (Object.prototype.hasOwnProperty.call(arguments[i], key)) {
        obj[key] = arguments[i][key];
      }

[nknapp]

I guess this proposal will ruin test-coverage, but maybe there is a way around that.

[mohd-akram]

Tried profiling this using the script in the linked issue. This PR is 6% faster than using Object.assign, and 12% faster than using Utils.extend. Note that the options object is already modified in this function in a couple places (options.ids and options.partials). It's also modified in the invokePartial function. My understanding is that options is a literal that comes from the template, so it's always new, and this function just augments that. There's no benefit to copying AFAICT.

[nknapp]

Ok, I can see what you say. I still think that side-effects are bad, but I can understand your motivations.

Its been a long time that I wrote this code, but I think the main reason to use "extend" here was clean code and avoiding side-effects, not some particular bad thing that would happen otherwise.

I wonder what the performance improvement is on a real-life template though.

[mohd-akram]

While profiling this again, I noticed one more place show up where there are unnecessary copies. I've also put #1994 here, since it's related.

[jaylinski]

Two checks are failing, but this is because of nodejs/node#52554.

Improvements will be also applied to master.