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.

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: don't use VM when not needed #2

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Performance: don't use VM when not needed #2

wants to merge 3 commits into from

Conversation

EdJoPaTo
Copy link

As this package assumes `strings` in JS ${ is needed to do logic.
When it is not included, logic is not needed -> no vm needed.

Within my projects I have many templates which do not use execution but always take time. As they can be used often this should take as little time as possible.

In order to check I used this for loop:

console.time('time')
for (let i = 0; i < 1000; i++) {
  compile('something lame without execution')
}
console.timeEnd('time')

With the optimisation I get this result: time: 1.275ms
Without the optimisation I get time: 3.614ms
While using this within telegraf-i18n this change alone improves the startup time of the bot (which recreates notifications in the users languages) from about 9s to 2.4s.

I also added the package.json files section and changed an Error to TypeError but as these changes are very small I don't think another PR is worth it.

As this assumes `strings` in JS ${ is needed to do logic.
When it is not included, logic is not needed -> no vm.
@dotcypress
Copy link
Owner

dotcypress commented Aug 16, 2019

@EdJoPaTo I think to fix performance issue we need to patch telegraf-i18n not compile-template, because template library don't make any sense with templateless input 😉

Happy to merge rest of changes.

UPD: new version of telegraf-i18n has been released.

@EdJoPaTo
Copy link
Author

EdJoPaTo commented Aug 17, 2019

When asking the question "what's the purpose of telegraf-i18n? What does it abstract for me?" it does not make sense to me to change that there.

In my opinion the template without a function in it is the simplest form of a template.
implementing this on the user side off this lib (telegraf-i18n) does not make sense to me as they just have some kind of template. how to handle the template effectively is not the job of the user, its the job of the template engine. telegraf-i18n has the job of getting the right template on the given language and resourceKey so it shouldn't be the task of it to optimise performance for something it isn't made for. (Especially as Telegraf-i18n only passes the user inputted template through to the template engine. "Its your job, handle that for me.")

Everyone that may want to use this library might want the same: fast templates with every kind of template they stick into it. But they have to do the same kind of optimisation which telegraf-i18n does again to use this library more efficiently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants