eejs is using blocking API :: Wont fix as it doesn't actually effect performance noticably.

[fourplusone]

readFileSync blocks the event loop until the file is read so it should not be used on every request: https://github.com/Pita/etherpad-lite/blob/7cf4510bf545927edead70931befb7d62ca5863c/src/node/eejs/index.js#L104
//cc @redhog

[redhog]

This is a hard one to solve, since all of EJS is using function calls with return values, not callbacks. eejs is justa function library for embedded javascript within a normal EJS template, and so must return the values that are to be used in the template.

[redhog]

The alternative would be a total rewrite of EJS into a non-blocking framework.

[jhollinger]

We could also switch to something besides ejs. There are quite a view node templating systems. Don't know which are non-blocking though.

[redhog]

This is alsp partly related to the node.js require() api being CommonJS synchronous modules, not http://wiki.commonjs.org/wiki/Modules/AsynchronousDefinition which can be solved by using http://requirejs.org/docs/node.html

[marcelklehr]

Unfortunately, it is still possible to make synchronous/blocking calls [...] such as [...] writeFileSync. Even if you avoid synchronous methods in your own code, it's still possible to inadvertently use an external library that has a blocking call. When you do, the impact on performance is dramatic.
Our initial logging implementation accidentally included a synchronous call to write to disc. [...] this one synchronous call caused a performance drop from thousands of requests per second to just a few dozen!

Source: http://engineering.linkedin.com/nodejs/blazing-fast-nodejs-10-performance-tips-linkedin-mobile#synchronous

[JohnMcLear]

@redhog any update on this? :|

[JohnMcLear]

<analphabet>blocking eejs is still a major bug, tho!
<JohnMcLear>Yep, we need to focus on core for a while, get developers deeply involved w/ the editor, not just some fancy UI thing around the editor
<JohnMcLear>Well it isn't a "major" issue for users though
<JohnMcLear>you can still accomplish a lot
<analphabet>everytime i think about it makes me shiver and i still can't believe it.
<analphabet>man, it's *blocking*!
<analphabet>readFileSync!
<JohnMcLear>hah
<analphabet>that should scare away every sensible node developer
<JohnMcLear>But how much does it affect performance/users?
<analphabet>greatly!
<analphabet>readFileSync called on every request!
<analphabet>boom -- performance brake
<JohnMcLear>isn't that only if you have maxAge == 0
<analphabet>it's still a performance shock
<analphabet>because node can't do ANYTHING while that file is read
<analphabet>no ruddy thing
<analphabet>no socket.io communication
<analphabet>no nothing
<JohnMcLear>hr
* analphabet hopes to have made his point clear...
<JohnMcLear>nope
<JohnMcLear>if caching is enabled why is it doing the readFileSync ?
<analphabet>it doesn't
<JohnMcLear>Then I'm still confused..
<JohnMcLear>It's not a performance shock if maxAge is != 0 right?
<analphabet>ah that'd be worth investigating
<JohnMcLear>Hah./
* JohnMcLear chuckles
<JohnMcLear>if you only make that call when you are in development mode that's fine..
<JohnMcLear>that's what we need to find out, if it's in production by default
<analphabet>shouldn't be too hard: add a console.warn('BlOCKING'); statement right before the blocking call :)
<JohnMcLear>try it and see ;)
...
<analphabet>JohnMcLear: heh
<analphabet>JohnMcLear: i set maxAge to 21600000
<analphabet>...
<analphabet>[2012-11-22 19:31:26.901] [WARN] console - BLOCKING! (src/node/eejs/index.js:123)
<analphabet>[2012-11-22 19:31:26.995] [INFO] http - 200, GET /
<analphabet>...
<analphabet>[2012-11-22 19:31:27.296] [INFO] http - 200, GET /locales.ini
<analphabet>[2012-11-22 19:31:32.041] [WARN] console - BLOCKING! (src/node/eejs/index.js:123)
<analphabet>[2012-11-22 19:31:32.051] [INFO] http - 304, GET /
<analphabet>...
<analphabet>[2012-11-22 19:31:32.275] [INFO] http - 304, GET /locales.ini
<analphabet>[2012-11-22 19:31:38.257] [WARN] console - BLOCKING! (src/node/eejs/index.js:123)
<analphabet>[2012-11-22 19:31:38.285] [INFO] http - 200, GET /p/test
<analphabet>...
<analphabet>[2012-11-22 19:31:38.488] [INFO] http - 404, GET /static/custom/pad.css
<analphabet>[2012-11-22 19:31:38.498] [WARN] console - BLOCKING! (src/node/eejs/index.js:123)
<JohnMcLear>ugh.
<analphabet>do'you mind trying the same on beta.etherpad.org?
<analphabet>(i dunno whether you've got caching enabled there, tho)
<JohnMcLear>will do now>
<JohnMcLear>yep I get lots of BLOCKING messages if I put console.warn("BLOCKING") at 123 on index.js

[JohnMcLear]

[19:24:34] Egil Moeller: Problem is, no one has suggested a better (E)EJS implementation that does not block.
[19:24:47] Egil Moeller: It is doable. It just means we'd have to write out own EJS compiler...
[19:52:53] John McLear: The bit that confuses me is why the cache doesn't trump the EEJS implementation?
[19:53:02] John McLear: Surely that's our quick win?
[19:53:55] Egil Moeller: Yeah... That's strange
[19:54:06] Egil Moeller: But: Can't we just hack caching of EJS _sources_?
[19:54:22] Egil Moeller: That should be easy, and mean that you get the async loads once at startup...
[19:55:45] John McLear: yea..
[19:56:01] John McLear: So I'm not going insane then, that's good!
[19:56:09] John McLear: Is that something you could put together relatively quickly?
[19:56:44] Egil Moeller: I guess it shouldn't be too hard no...
[19:56:59] Egil Moeller: As in probably a day's work, at max

[JohnMcLear]

We're still yet to hit this issue as being a performance bottle neck despite lots of load testing. With that in mind we're not too concerned about the current eejs implementation.. If anyone wants to share an opinion please go for it :)

[devoidfury]

How much third-party code depends on ejs? It may be better to switch to a different engine that handles async well, unless it'll break a lot of code.

Alternatively, we could just cache the templates in memory and only read them from disk once.

[devoidfury]

Actually, I see a quick performance win in eejs with caching the templates; let me take a stab at that and I'll get back to you.

[muxator]

This is still true as of today:

etherpad-lite/src/node/eejs/index.js

Line 117 in c966480

var template = '<% e._init(__output); %>' + fs.readFileSync(ejspath).toString() + '<% e._exit(); %>';

Just for info, as part of #3540, @raybellis already migrated a big part of the codebase to async/await. That work will be part of 1.8. Sadly, eejs is stuck in the past.

[JohnMcLear]

Afaik w/ the recent minification / caching efforts we're in a pretty good place. I'm not sure we will get much gains from this change?

[muxator]

Only testing could tell us.

I would leave this open, because the issue is different than minification/caching and is a good PITA to increase the desire to modernize the code base.