Asynchronous API #41
Comments
|
Bump |
|
The thing is I don't think it would help very much. Once a template is loaded it is cached for the duration of the process. It is only reloaded if the file's modification times are newer than the cached version. So those sync methods are never really used in production, only the very first request. The only one that is used every request is statSync, so the chance of a scalability problem is pretty low. It's possible we need to make that async though, but that would require about the same amount of refactoring. Not a huge amount, but isn't a quick change. |
|
In nodejs 0.9 there's a proper watchFs for all platforms, not just linux. That should make the stat on every access be replaceable with inotify/kqueue/etc. |
|
@jbergstroem That's great. I suppose we could use that to set flags in the Environment object to reload templates. That's why I love node. It's so freakin' easy to do async stuff. |
|
We're doing similar stuff for vhosts in express (load entire site if changed) and eagerly await this too 👍 |
|
In a production environment it is common to have a flag that assumes the app should not constantly stat() assets and templates just in case they have changed, because they are only changed at deployment time (when the app is restarted). That would be handy, and negative work almost (: |
|
@boutell Indeed. To take that even further - you should cache your templates at a higher tier. Not saying it's one or the other; most preferably both. I recall both django and rails doing something similar with it's production/development flags. On the other hand, that also shows that its probably not up to the template engine to do rather than exposing an way of handling it. |
|
@boutell I'm trying to avoid having to set a dev/prod flag if possible. I believe that using watchFs would mitigate the issue though, because it has zero performance impact because the filesystem events are coming from the kernel. Does that sound good? |
|
I think having the flag available makes a lot of sense, I wouldn't call it Or, yeah, you could ask the OS to watch the files for you. I'm not sure On Thu, Jan 24, 2013 at 1:53 PM, James Long notifications@github.comwrote:
Tom Boutell |
|
Another thought on this - I'm writing a Loader that pulls the template from a mongo db. |
|
Hmm, interesting. We just got used to the fact that Node's template On Fri, Feb 15, 2013 at 4:14 PM, James Long notifications@github.comwrote:
Tom Boutell |
|
Haven't seen anyone start on this yet, so I've begun working on it here - (https://github.com/devoidfury/nunjucks/tree/async_templates). There's still a long way to go, but it's a start. An asynchronous API will break existing code, unless we use a flag and maintain both styles. |
|
@boutell Our main focus should be loading templates asynchronously. So you'll still have to load all your data up front and pass it into the templates (I personally don't see this as a problem, otherwise your putting too much logic in your templates). This change is mostly a backend change and most people won't notice if they are just using the express integration. @devoidfury It will change the API, yes, but we can update the express integration to make it work seamlessly for people who aren't using the API directly, which is probably most people. Thanks for starting this, it looks good. This will also help the HttpLoader in the browser because right now it has to use sync ajax, which is yucky. |
|
@jlongster Already taken care of express integration and the HttpLoader in my branch. I'm working on extends, includes, and updating the tests at the moment. |
|
@devoidfury I'm considering changing how precompiled templates are integrated, and the async API would be required with the new API I'm thinking of, so I'll make this a higher priority. This would require all render calls to be async, right? So instead of this: var tmpl = nunjucks.env.render('foo.html');We now have: nunjucks.env.render('foo.html', function(tmpl) {
// ...
});This is quite a big change, and we'll probably have to keep it behind a flag for a while to not break existing apps. Also, can you guys think of any major problems this causes with any integration with frameworks, like backbone, ermber, etc? All of the other templating libraries seem to be synchronous, so this worries me a lot. If other frameworks expect it to be sync, we have a problem. |
|
Interesting... you could possibly have an optional callback, that is used if present. Otherwise return synchronously as before? |
|
Now that I finished autoescaping and custom tags API, this is the next big feature to tackle. I'll spend some time on this next week. I agree that we can probably make sync and async version work at the same time, but that will require some more research. |
|
I'm really excited about this feature; made some good progress on my branch, but I probably wouldn't merge it at this point as it's written as a one-way breaking change (and unfinished). The only async templating libraries I've found so far that aren't abandoned are QEJS, node-blue, and kiwi. Might look to them for inspiration. Kiwi seems to be express-compatible; we may look for other usages of it to see what kind of real-world challenges await an async implementation. |
|
I'm somewhat worried about making something like this optional, because it might things too confusing -- writing filters, for example, shouldn't involving always handling an optional callback. I'm wondering though, if we can enable both styles at once, if we should deprecate the sync style and drop it later on. I don't have a great grasp on how many people are using nunjucks, but I don't think it's a huge amount. At least, there isn't a big repo of nunjucks filters/extensions yet, so now would be a good time to enforce this if we are going to. In fact, I'm planning on making the next release v0.2.0 (or maybe even higher) because it include autoescaping, a custom tag API, and possibly this async stuff. It would be a good time to introduce such a big change. |
|
We're relying on nunjucks for Apostrophe, so we would definitely appreciate not having a breaking change this big happen on a patchlevel version number. But as long as you choose the version number well it should work out. |
|
@jlongster My take is to get it in sooner than later. Bumping to 0.2.0 sounds reasonable; the autoescaping will probably make a lot of people audit their stuff anyway. Will this also rely on the node 0.10 watchFs (read: a working cross-platform watchFs implementation)? |
|
I have a concrete use case for an async filter API: https://github.com/brianloveswords/node-htmlsanitizer is a node module that hits a Bleach.py REST endpoint to sanitize HTML. I'd love to be able to add a | bleach filter to user input and just have it work. A man can always dream, right? |
|
@jbuck yeah, that's a pretty good one. In the next week or two I plan on setting down and thinking hard about this. I have some ideas for other API improvement too, and I will post them to the mailing list. If you are interested in this discussion, please join the mailing list (https://groups.google.com/forum/?fromgroups#!forum/nunjucks). When we nail down specific actions we can pick this bug back up. |
|
I for one would love asynchronicity. Especially around custom blocks :) |
|
We have a strong preference for being able to render "partials" (templates So we usually wind up including a function called "partial" in the data We also tend to write convenience functions that do more logic (although it So my concern is that it remain possible to use nunjucks in a synchronous I also have to ask if all of these asynchronously loaded templates really On Sun, Jul 7, 2013 at 6:24 AM, Michael Robinson
Tom Boutell |
|
I'm looking into this finally, and hope to have something implemented soon. I agree that it's useful to have both APIs, but I'm concerned at how much that will split filters/extensions. More research is needed. |
|
Personally I'm interested in being able to use asynchronous data fetching within custom tag processing code, although I feel at the node community as a whole would love to have the async option for the flagship templating engine ;) |
|
It seems like there a few things going on in this issue. Originally, the discussion was about using watchFs instead of stat'ing every hit to see if the templates have changed. I believe we can do that without the big async overhaul. We should probably just file another issue for that, and focus on making all rendering async. I'm pickup the work back up, starting with what devoidfury has done, and will focus on making the rendering APIs async. I really don't want to maintain 2 different APIs and split the code into sync and async versions. I think the biggest pain point will be filters, since that's what most people have written. We can probably detect when a filter is async, so that we can allow both sync and async filters, which will solve that problem. |
|
In addition to filters, we also inject functions whose output will in some For example:
{{ aposArea(page.slug + ':sidebar', page.areas.sidebar) }}
It doesn't seem likely that it would still be possible to do this in an The ability to inject a function that executes some fairly elaborate logic, On Mon, Jul 22, 2013 at 4:02 PM, James Long notifications@github.comwrote:
Tom Boutell |
|
I've hit an edge case that won't be possible without converting all for loops to be async: That's actually possibly right now, but only because everything is properly implemented without concerns of each other. This isn't possible when async because rendering blocks are async, and the |
|
What is the expected behavior there? The last iteration's value for the On Wed, Jul 24, 2013 at 5:08 PM, James Long notifications@github.comwrote:
Tom Boutell |
|
You could actually override the block in a sub-template and it would just be evaluated like normal for every iteration. I don't think anyone does this (nor should they), but it used to just work. I actually think I've hit some serious problems with for loops, and I might have to make them async. Still looking into it. |
|
Just an update. I've converted most things to be async. I'm having trouble with macros though, because when you call a macro, I don't know if it's a macro or normal function at compile time (so I don't know if I should generate async code or not). It'd be easy to track macro names, except you can do The much bigger issue I've hit is the performance degradation. On average there's a 25-30% perf hit across the board for being async. I think it's mainly because I ended up having to make loops async, so they aren't a normal C-style loop anymore. There's also a lot more function calls in general. That's not acceptable to me. I think I'm going to change my approach and make async behavior "opt-in". The high-level API will always be async, but within nunjucks you'll have to be more explicit about when you are doing async stuff. This probably means a different (another example: I think I'm going to force devs to tell the compiler which filters/extensions are async, so that it knows at compile-time what to do) |
|
I think this makes a lot of sense for something that runs as intensively on I recently ported an old mandelbrot set plotter from Java to JavaScript to I spoke with some optimization experts who told me to skip the async When compared to threads and I/O is involved, async is indeed fast. But in On Thu, Aug 1, 2013 at 7:33 PM, James Long notifications@github.com wrote:
Tom Boutell |
|
In most cases, async patterns ruin the interpreter's ability to optimize basically anything that the code does because types can't be determined (or narrowed down) statically. In synchronous patterns, return types can be inferred (or aren't needed) and type checks can be eliminated in many places. That's not the case in async code, and most of the time code in nested callbacks will back on completely unoptimized bytecode (in spidermonkey, you'd fall back on the raw AST interpreter since it's faster to run against the AST than it is to keep profiling for IonMonkey). Especially with string concatenation (the vast majority of what nunjucks does), the JS interpreter can do a ton of under-the-hood optimizations when the code is synchronous. I think in any circumstance where nunjucks isn't calling out to non-nunjucks (unjucked?) code, it will be faster to simply be synchronous and exposed as asynchronous. The only time where async code would be conceivably faster than synchronous code is where user-provided globals, filters, and functions in the context take substantial amounts of time to execute (DB calls, heavy async computation, calls to other processes, etc.).
I'd be curious if, during precompilation, you could actually open that file (precompiled templates will always exist) and just look it up. If you do a basic first pass before you generate any of the output, you could get the list of exported macros and blocks. Maybe the answer in the short term is to make async available only to precompiled templates? You could do a lot more at compile time to ease the burden on the runtime. Perhaps you could set some kind of |
|
I'll reply more tomorrow, but note that the only reason I'm interested in async is to allow more powerful interactions with async-only libraries, like db/http/fs calls, etc. I am definitely not doing it for performance, and I knew there would be a hit, I just didn't know how much. |
|
I really like the suggestion to make it more obvious whether an action is going to be sync or async, and being required to specify whether a component is sync/async. Explicit declarations all the way. Michael Robinson On 2/08/2013, at 11:33 AM, James Long notifications@github.com wrote:
|
|
You're right - I'm rooting for async ONLY for my custom blocks - I want to pull content from the db. Everything else is non-blocking so async is unnecessary overhead there. Michael Robinson On 2/08/2013, at 12:05 PM, Tom Boutell notifications@github.com wrote:
|
I like where you are going, but I think that's too limiting. I can see that being very confusing to a new developer to nunjucks, and it would be hard to explain why. However, I'm all for adding limitations to async code. I think only top-level filters/tags will be able to be async, because you can't do anything async underneath a synchronous form. This solves a lot of problems (macros won't be able to do async stuff, there will be async for/if variants, most code continues to be sync, etc) |
|
Can you include a file from inside a 'for' or 'if'? Would that affect this On Fri, Aug 2, 2013 at 9:24 AM, James Long notifications@github.com wrote:
Tom Boutell |
That's a good point, but yes, you should be able to. Every |
|
Three cheers for being explicit - that makes my job a lot easier since On Fri, Aug 2, 2013 at 9:53 AM, James Long notifications@github.com wrote:
Tom Boutell |
|
@boutell Yeah, and most people really should do it that way. Async-ness was a featured requested enough that I think we'll see some neat applications of it, but it's bad practice to start loading everything in the template. (side node: you referenced the problem that nunjucks can't include relative templates, and after thinking about it a bit more I'm open to the idea as long as it doesn't complicate precompiled templates too much. feel free to file an issue for it) |
|
k guys. I think the async work is stabilizing. As I said before, the default in nunjucks is still synchronous. However, the internals have been refactored a bit to allow you to introduce async behavior, but it comes with some caveats. The result is that the performance is basically the same, but you can still use async in most places. Right now, the key part is filters. You can introduce async filters like this: var fs = require('fs');
var env = new nunjucks.Environment();
env.addFilter('getContents', function(path, cb) {
fs.readFile(path, cb);
}, true);Note the last argument You can just use it normally in a template: {{ "path/to/file/this/is/a/bad/example" | getContents }}Now for the caveats: you can't use anything async inside the {% ifAsync tmpl %}
{{ tmpl | getContents }}
{% endif %}This has a nice feature where it is obvious where async stuff is happening, too. Note that you still close it with normal The last thing is template loaders. Bear with me here. Those can be async, too, but adding an HOWEVER, the important change is that So, the very original minor issue of converting to fs.watch has been fixed. In addition, I completely re-hauled the internals of the compiler and implemented an AST transformer to generate async-friendly code. |
|
Oh, forgot to mention that I've converted all the tests to use the async API and they are all passing. The |
|
Now that I think about it, since I have the list of async filters at compile-time, it wouldn't be that hard to convert |
|
I think that is best since frontend devs would have a heck of a time Still wondering what happens if a macro uses a filter and contains if
|
|
Most people won't use async and won't care about any of this, I think, so the burden is placed on those who want to do this. However, it would be nice to get rid of
Macros don't allow async expressions in them, because macros look like function calls and I can't reliable detect whether or not to call it async or not (if you passed in normal functions, they should be called sync). I don't think this will be a big deal. You shouldn't be doing that much async stuff anyway. |
|
Lots of tests now: https://github.com/jlongster/nunjucks/blob/async/tests/compiler.js#L210 |
|
I'm pretty much done here. Precompiled templates and extensions can't use async yet, but that wouldn't be hard to add. I'll do that later. Can someone volunteer to test the async branch with some complicated templates? I want to make sure there are no regressions. It'd be great if you want to write some async filters, too. |
|
A while ago I played around with http chunked responses to make some pages load seemingly "faster" without ajax. I used the usual method of sending back scripts that would pop rendered portlets into place on the page. I haven't been following this thread very closely, but I wonder what your thoughts are on using this with http 1.1 chunked? |
|
I also have a question: am I now able to make my custom blocks async? |
|
@mkoryak The internal infrastructure is there to do anything async, so you could kind of do what you are talking about. I carefully exposed the async control, and we can tweak how it works over time depending on use cases. If you are wanting to control the output buffer, you can't do that, but I would be open to discussing how we can make that work. @faceleg You definitely will be able to, but not yet. I haven't exposed async control to the extensions but it will be easy since the internal code is ready for it. |
|
@faceleg Ok, I implemented async extensions. Do you mind helping test it? First, make sure your existing sync extensions still work. Then, test the new async work. Instead of returning Here's an example: function FooExtension() {
this.tags = ['foo'];
this._name = 'FooExtension';
this.parse = function(parser, nodes) {
var tok = parser.nextToken();
var args = parser.parseSignature(null, true);
parser.advanceAfterBlockEnd(tok.value);
var body = parser.parseUntilBlocks('endfoo');
parser.advanceAfterBlockEnd();
return new nodes.CallExtensionAsync(this, 'run', args, [body]);
};
this.run = function(context, baz, body, cb) {
cb(null, 'baz: ' + baz + ', body:' + body());
};
}{% foo "baz" %} some content here {% endfoo %}One thing to note is the content functions, like this.run = function(context, baz, body, cb) {
body(function(err, res) {
cb(null, 'baz: ' + baz + ', body:' + res);
}
}; |
|
I can't wait to start working with this, hopefully later tonight, definitely within the next few days. Will report back. |
|
I've tweaked the loop constructs. We should go ahead and provide to async iteration primitives, one which iterates sequentially and one which executes all the items in parallel. I renamed Depending on the async functionality you are using inside the loop, you choose which one you want. I think most of the time you probably want This will be clearly documented. |
|
@jlongster works like a charm, thank you so much! |
|
This has been released in 1.0, and the node loader now uses fs.watch. |
I know that nunjucks is a direct port of jinja2, so probably due to this internally it is using some sync methods of nodejs to access files, etc. However I think that having an asynchronous API would bring better performance. For example inside FileSystemLoader there are calls to fs.existsSync, fs.readFileSync and fs.statSync. This blocks the whole javascript execution so it causes a scability problem.
I know that this could be a big change, but would be a big step forwards.
The text was updated successfully, but these errors were encountered: