Perform replacement in one pass of the output #67
Conversation
TikiTDO
force-pushed
the
feature/replacer
branch
4 times, most recently
from
ed2c041 to
7813180
Compare
src/GenerateTemplateFiles.ts
Outdated
| // Create a function to apply the transformations in one go | ||
| const regexEscape = (text: string) => text.replace(/([^a-zA-Z0-9_])/g, '\\$1'); | ||
| const replacerLookup: Record<string, string> = {}; | ||
| const replacerRegexBase = replacers | ||
| .map((replacer: IReplacer) => { | ||
| replacerLookup[replacer.slot] = replacer.slotValue; | ||
| return regexEscape(replacer.slot); | ||
| }) | ||
| .join('|'); | ||
| const replacerRegex = new RegExp(`^${replacerRegexBase}$`, 'g'); | ||
| const replacer = (text: string) => text.replace(replacerRegex, (slot) => replacerLookup[slot]); |
There was a problem hiding this comment.
Are you trying to remove replace-string as a dependency and write you own code? I don't understand the benefit to adding this code over the three lines that is already there:
replacers.forEach((replacer: IReplacer) => {
output = replaceString(output, replacer.slot, replacer.slotValue);
});
|
The issue with With the addition of the underline case values that means each string has to be iterated through 12 times for every single token. It's a few milliseconds for a small file with one or two tokens, but if you were trying to generate a really big template with a bunch of tokens it might actually start to matter. The implementation above will iterate through the output string once, and replace each appropriate value as it finds it. It is 12 * # of Tokens times faster, minus the time to compile the regex, which should be trivial for a mostly static regex like this one. |
|
For more concrete numbers, I did a test with three approaches. The simple The trie is more effective when working with larger files, while the simple case rips through smaller files. Both were around 7-12x faster than the |
TikiTDO
force-pushed
the
feature/replacer
branch
from
7813180 to
0411f4b
Compare
package.json
Outdated
| @@ -1,105 +0,0 @@ | |||
| { | |||
There was a problem hiding this comment.
Did you mean to delete this file?
There was a problem hiding this comment.
Ah whoa, how did that happen.
There was a problem hiding this comment.
I restored the file if you wanted to play around with the 3 approaches. Looks like I deleted it instead of an package-lock.json while I was cleaning up.
|
Thanks for the insight why you are changing out |
|
No rush. The other two PRs were the really important ones. This one is just a performance optimization for super extreme edge cases. Really just idle work while I was waiting for a build. |
This changes the transform pass of the generator to run in one pass of the output string, instead of using string replacer. It might make sense to just make something like an
applyTransformersToString, or whatever name you might like, and put it in a util file. If you have an opinion on where it should go I'd be happy to move it.