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.

Sign up for GitHub

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

Jump to bottom

Improve rendering performance #1993

Merged
merged 1 commit into from
Sep 3, 2024
Merged

Improve rendering performance #1993

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/handlebars/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ export function moveHelperToHooks(instance, helperName, keepHelper) {
if (instance.helpers[helperName]) {
instance.hooks[helperName] = instance.helpers[helperName];
if (!keepHelper) {
delete instance.helpers[helperName];
// Using delete is slow
instance.helpers[helperName] = undefined;
}
}
}
11 changes: 0 additions & 11 deletions lib/handlebars/internal/create-new-lookup-object.js
View file
Open in desktop

This file was deleted.

32 changes: 15 additions & 17 deletions lib/handlebars/internal/proto-access.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,32 +1,30 @@
import { createNewLookupObject } from './create-new-lookup-object';
import { extend } from '../utils';
import logger from '../logger';

const loggedProperties = Object.create(null);

export function createProtoAccessControl(runtimeOptions) {
let defaultMethodWhiteList = Object.create(null);
defaultMethodWhiteList['constructor'] = false;
defaultMethodWhiteList['__defineGetter__'] = false;
defaultMethodWhiteList['__defineSetter__'] = false;
defaultMethodWhiteList['__lookupGetter__'] = false;

let defaultPropertyWhiteList = Object.create(null);
// Create an object with "null"-prototype to avoid truthy results on
// prototype properties.
const propertyWhiteList = Object.create(null);
// eslint-disable-next-line no-proto
defaultPropertyWhiteList['__proto__'] = false;
propertyWhiteList['__proto__'] = false;
extend(propertyWhiteList, runtimeOptions.allowedProtoProperties);

const methodWhiteList = Object.create(null);
methodWhiteList['constructor'] = false;
methodWhiteList['__defineGetter__'] = false;
methodWhiteList['__defineSetter__'] = false;
methodWhiteList['__lookupGetter__'] = false;
extend(methodWhiteList, runtimeOptions.allowedProtoMethods);

return {
properties: {
whitelist: createNewLookupObject(
defaultPropertyWhiteList,
runtimeOptions.allowedProtoProperties
),
whitelist: propertyWhiteList,
defaultValue: runtimeOptions.allowProtoPropertiesByDefault
},
methods: {
whitelist: createNewLookupObject(
defaultMethodWhiteList,
runtimeOptions.allowedProtoMethods
),
whitelist: methodWhiteList,
defaultValue: runtimeOptions.allowProtoMethodsByDefault
}
};
Expand Down
32 changes: 14 additions & 18 deletions lib/handlebars/runtime.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,25 +74,18 @@ export function template(templateSpec, env) {
}
partial = env.VM.resolvePartial.call(this, partial, context, options);

let extendedOptions = Utils.extend({}, options, {
hooks: this.hooks,
protoAccessControl: this.protoAccessControl
});

let result = env.VM.invokePartial.call(
this,
partial,
context,
extendedOptions
);
options.hooks = this.hooks;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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];
      }

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

options.protoAccessControl = this.protoAccessControl;

let result = env.VM.invokePartial.call(this, partial, context, options);

if (result == null && env.compile) {
options.partials[options.name] = env.compile(
partial,
templateSpec.compilerOptions,
env
);
result = options.partials[options.name](context, extendedOptions);
result = options.partials[options.name](context, options);
}
if (result != null) {
if (options.indent) {
Expand Down Expand Up @@ -247,8 +240,9 @@ export function template(templateSpec, env) {

ret._setup = function(options) {
if (!options.partial) {
let mergedHelpers = Utils.extend({}, env.helpers, options.helpers);
wrapHelpersToPassLookupProperty(mergedHelpers, container);
let mergedHelpers = {};
addHelpers(mergedHelpers, env.helpers, container);
addHelpers(mergedHelpers, options.helpers, container);
container.helpers = mergedHelpers;

if (templateSpec.usePartial) {
Expand Down Expand Up @@ -428,16 +422,18 @@ function executeDecorators(fn, prog, container, depths, data, blockParams) {
return prog;
}

function wrapHelpersToPassLookupProperty(mergedHelpers, container) {
Object.keys(mergedHelpers).forEach(helperName => {
let helper = mergedHelpers[helperName];
function addHelpers(mergedHelpers, helpers, container) {
if (!helpers) return;
Object.keys(helpers).forEach(helperName => {
let helper = helpers[helperName];
mergedHelpers[helperName] = passLookupPropertyOption(helper, container);
});
}

function passLookupPropertyOption(helper, container) {
const lookupProperty = container.lookupProperty;
return wrapHelper(helper, options => {
return Utils.extend({ lookupProperty }, options);
options.lookupProperty = lookupProperty;
return options;
});
}
Loading