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

Treat formats and buffers as interchangeable when they are mixed together #511

Merged
merged 1 commit into from
Nov 3, 2014

Conversation

sethkinast
Copy link
Contributor

During the compiler's optimization pass, "format" globs (basically whitespace) are stripped out. The remaining strings, "buffers", are concatenated into a single entity.
If whitespace compression is disabled, formats and buffers are all written, but the two types are not concatenated together. Because most code is broken up by newlines, this results in highly-inefficient chunking of writes that-- in the case of large templates-- causes Dust to exceed the call stack limit.

This patch adds dust.config.whitespace (set by default for now to false for backwards compatibility; by default there is zero change with or without the patch) to toggle the whitespace stripping behavior. When the flag is set to true (allow whitespace), the new format/buffer concat logic is invoked.

Closes #498

@shakyShane
Copy link

@jimmyhchan
Copy link
Contributor

Can this work without changing the grammar. It's not back compat (what
about folks relying on our parser) and it makes more sense to keep data
parsed but combine it in the compiler.

I'm probably missing how combining buffers solve the issue.

What about the idea for a pre compilation step.

@sethkinast
Copy link
Contributor Author

It's not as pretty, but let me see if I can maintain perf.

We lose a lot of speed at the compilation step by manually combining nodes. I get the desire to maintain grammar, though.

I'm not sure the value that we get by separating out the newline from any following space (except for backcompat)

@sethkinast
Copy link
Contributor Author

OK, we don't lose that much speed. About 8%. Grammar is now backwards-compatible.

dustc now has a --whitespace flag you can set to true to preserve whitespace on the command-line.

Default behavior is still dust.config.whitespace == false

…ther.

During the compiler's optimization pass, "format" globs (basically whitespace) are stripped out. The remaining strings, "buffers", are concatenated into a single entity.
If whitespace compression is disabled, formats and buffers are all written, but the two types are not concatenated together. Because most code is broken up by newlines, this results in highly-inefficient chunking of writes that-- in the case of large templates-- causes Dust to exceed the call stack limit.

This patch adds dust.config.whitespace (set by default for now to false for backwards compatibility; by default there is zero change with or without the patch) to toggle the whitespace stripping behavior. When the flag is set to true (respect whitespace), the new format/buffer concat logic is invoked.

Also adds --whitespace=true|false (default false) flag to dustc.

Closes linkedin#498
@prashn64
Copy link
Contributor

lgtm, one more look from @jimmyhchan

};

// Directive aliases to minify code
dust._aliases = {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need these public?
edit: nvm yes you do.. silly compiler.

@jimmyhchan
Copy link
Contributor

the minified stuff looks good thanks for that.
👍 to pull. Only slight concern for me is figuring out how this affects the matrix of folks who (had turned whitespace compression off/explicitly turned it on, are on previous/current versions of Dust compiler/runtime). I believe this will be completely back compat but want to make sure and if it needs some work then we should document what this is.

prashn64 added a commit that referenced this pull request Nov 3, 2014
Treat formats and buffers as interchangeable when they are mixed together in order to prevent stack overflows with large templates and add a whitespace flag to dust.config
@prashn64 prashn64 merged commit 695441d into linkedin:master Nov 3, 2014
@sethkinast sethkinast deleted the whitespace-optimization branch November 4, 2014 00:26
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.

Cannot render large documents with white-space suppression disabled
4 participants