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 degradation for partials since v4.3.0 #1598
Comments
While I'm unable to verify that this fiddle actually reproduces this behavior, this setup should hopefully be able to reproduce the issue if needed: |
Reintroduce assigning env.partials to container.partials if no options.partials is set. Also allow assigning options.partials to container.partials if only that one exists. This solves handlebars-lang#1598
Reintroduce assigning env.partials to container.partials if no options.partials is set. Also allow assigning options.partials to container.partials if only that one exists. This solves handlebars-lang#1598
I don't think I intended this change for partials. But I didn't understand why the "merge" function was doing all those extra struff, so I just removed it for the sake of cleaner code. The first thing I would like to have in any case, is a test-case that reproduces the bug, so this does not happen again. I'm not sure how yet this can be done exactly, but we should check the the compile-function is only called once when the template is executed twice. As for the fix: I would like a solution that makes clear why the partials-object is not simply merged. The safe solution is to re-introduce the If you would like to do that, go ahead. If not, I can do it myself. I am sorry for introducing this issue and will fix it if necessary. |
@nknapp Thank you for the feedback. I've updated #1600 now with a compile-count test (that will fail on v4.3.0+, but succeeds on this branch) and reintroduced the merge-function + comment instead of the if/else I had. I added the test to spec/compile.js, but if you'd rather have it as part of partials or runtime feel free to change it. |
Keep handlebars at 4.2.x because handlebars-lang/handlebars.js#1598 breaks our test suite, and simplebar at 4.2.x because of Grsmto/simplebar#406. Upgrade everything else. Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Keep handlebars at 4.2.x because handlebars-lang/handlebars.js#1598 breaks our test suite, and simplebar at 4.2.x because of Grsmto/simplebar#406. Upgrade everything else. Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Reintroduce assigning env.partials to container.partials if no options.partials is set. Also allow assigning options.partials to container.partials if only that one exists. This solves #1598
Reintroduce assigning env.partials to container.partials if no options.partials is set. Also allow assigning options.partials to container.partials if only that one exists. This solves #1598
Keep handlebars at 4.2.x because handlebars-lang/handlebars.js#1598 breaks our test suite, and simplebar at 4.2.x because of Grsmto/simplebar#406. Upgrade everything else. Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Hi there,
We got an issue where upgrading handlebars beyond v4.2.1 introduces a major performance-impact on our site. We use handlebars server-side and since v4.3.0 partials do not cache their compile-function properly it is leading to a lot of overhead in parsing & building the compile functions again and again. (Total response-time goes from about 75ms to 250ms on the average request)
Underlying issue:
With v4.3.0+ any non-precompiled partials registered with handlebars.registerPartial do not cache their compiled value beyond the current template scope, causing them to be recompiled every time they are used for the first time within a template.
The cause:
I've narrowed it down and the change that is causing it is this line:
2078c72#diff-cccd9fbbd9d8ac0bff571e9c8b0ee9deL164-R159
The practical implications is that when no options.partials exists, handlebars still creates a shallow-copy of env.partials, while before container.partials would be set to reference the env.partials directly.
I haven't been able to prove exactly why this happens, but my assumption is that as the partials are not compiled at the time of the shallow-copy, the env.partials contains strings which never gets updated since strings are not copied by reference (thus when container.partials['my-partial'] is updated to the compiled function, the original env.partials reference is not affected)
Proposed solution:
(Note: I'm assuming removing the possibility of directly assigning env.partials to container.partials was not an intentional change to prevent the security issues in v4.2.1)
In runtime.js change L161
container.partials = Utils.extend({}, env.partials, options.partials);
into
It's not a perfect solution, as if there was both options.partials and env.partials defined it would still shallow-copy env.partions, but this should be more consistent with pre v4.3.0 behavior and even speed up the runtime performance as it no longer needs to loop through and shallow-copy env.partials if no options.partials are defined.
The text was updated successfully, but these errors were encountered: