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

Evaluate and cache text sections properly #1083

Merged
merged 1 commit into from Jun 13, 2014

Conversation

Projects
None yet
2 participants
@daffl
Contributor

daffl commented Jun 12, 2014

This pull request fixes #1065. Caching was done under the assumption that in a text sub section all expressions contain the same content making a simple template like the following fail:

<h2 class="{{#shown}}foo{{/if}} test {{#shown}}bar{{/shown}}"></h2>

The solution is to cache the evaluators for normal property lookups (like {{style}}) to improve performance but always create the evaluator when the mode is set (which means that there potentially is a different subsection).

@daffl daffl added this to the 2.1.2 milestone Jun 12, 2014

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jun 12, 2014

Contributor

Don't remove it if it kills performance. Let's figure out a way to keep it he same.

Sent from my iPhone

On Jun 12, 2014, at 5:11 PM, David Luecke notifications@github.com wrote:

This pull request removes the the evaluator caching for text section from can.stache which fixes the original issue #1065. Caching was done under the assumption that in a text sub section all expressions contain the same content making a simple template like the following fail:

The only solution I could find was removing the evaluator caching which does slow down performance for the visual benchmark.

You can merge this Pull Request by running

git pull https://github.com/bitovi/canjs stache-truthy-attributes-1065
Or view, comment on, or merge it at:

#1083

Commit Summary

Adding tests for truthy stache attributes (#1065).
Removing scope.__cache and updating test for #1065.
File Changes

M view/scope/scope.js (1)
M view/stache/mustache_core.js (9)
M view/stache/stache_test.js (12)
Patch Links:

https://github.com/bitovi/canjs/pull/1083.patch
https://github.com/bitovi/canjs/pull/1083.diff

Reply to this email directly or view it on GitHub.

Contributor

justinbmeyer commented Jun 12, 2014

Don't remove it if it kills performance. Let's figure out a way to keep it he same.

Sent from my iPhone

On Jun 12, 2014, at 5:11 PM, David Luecke notifications@github.com wrote:

This pull request removes the the evaluator caching for text section from can.stache which fixes the original issue #1065. Caching was done under the assumption that in a text sub section all expressions contain the same content making a simple template like the following fail:

The only solution I could find was removing the evaluator caching which does slow down performance for the visual benchmark.

You can merge this Pull Request by running

git pull https://github.com/bitovi/canjs stache-truthy-attributes-1065
Or view, comment on, or merge it at:

#1083

Commit Summary

Adding tests for truthy stache attributes (#1065).
Removing scope.__cache and updating test for #1065.
File Changes

M view/scope/scope.js (1)
M view/stache/mustache_core.js (9)
M view/stache/stache_test.js (12)
Patch Links:

https://github.com/bitovi/canjs/pull/1083.patch
https://github.com/bitovi/canjs/pull/1083.diff

Reply to this email directly or view it on GitHub.

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Jun 12, 2014

Contributor

I just realized that we actually can cache normal property lookups (that don't have the mode set). Fixes the test and keeps the spinning circle performance the same as before.

Contributor

daffl commented Jun 12, 2014

I just realized that we actually can cache normal property lookups (that don't have the mode set). Fixes the test and keeps the spinning circle performance the same as before.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jun 12, 2014

Contributor

Thanks!

Sent from my iPhone

On Jun 12, 2014, at 6:35 PM, David Luecke notifications@github.com wrote:

I just realized that we actually can cache normal property lookups (that don't have the mode set). Fixes the test and keeps the spinning circle performance the same as before :-)


Reply to this email directly or view it on GitHub.

Contributor

justinbmeyer commented Jun 12, 2014

Thanks!

Sent from my iPhone

On Jun 12, 2014, at 6:35 PM, David Luecke notifications@github.com wrote:

I just realized that we actually can cache normal property lookups (that don't have the mode set). Fixes the test and keeps the spinning circle performance the same as before :-)


Reply to this email directly or view it on GitHub.

@daffl daffl changed the title from Remove evaluator caching for text section to Evaluate and cache text sections properly Jun 12, 2014

daffl added a commit that referenced this pull request Jun 13, 2014

Merge pull request #1083 from bitovi/stache-truthy-attributes-1065
Evaluate and cache text sections properly

@daffl daffl merged commit 9a52492 into master Jun 13, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@daffl daffl deleted the stache-truthy-attributes-1065 branch Jun 13, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment