Edgewall Software

Opened 3 years ago

Closed 3 years ago

Last modified 3 months ago

#12639 closed enhancement (fixed)

Integrate Jinja2 branch — at Version 7

Reported by: Christian Boos Owned by: Christian Boos
Priority: high Milestone: 1.3.2
Component: rendering Version: 1.3dev
Severity: critical Keywords: jinja2 performance +testing
Cc: Branch:
Release Notes:

The template engine used by Trac is now Jinja2. Genshi templates rendering in plugins will be supported until Trac 1.5.1.

API Changes:

Major changes in trac.util.html, trac.web.Chrome, trac.web.RequestFilter, deprecation of trac.web.ITemplateFilter.

See details in TracDev/ReleaseNotes/1.3#Developer-visiblechanges.

Description

The work on the switch to the Jinja2 engine is mostly complete on the experimental branch cboos.git@jinja2.

Like explained in TracDev/Proposals/Jinja, this change will adequately solve the performance issues we had with Genshi: pages render from 3x to 10x faster, at a fraction of the cost in memory.

This ticket is about the finalization and review, and the merge of the changes on trunk.

Besides the merge and catching up with the changes that happened on trunk since the merge-base (r14499), the main open points are:

  • get rid of the j* prefix used for the Jinja2 pages and find another mechanism to support both Jinja2 templates and legacy Genshi templates
  • test l10n for template pages in plugins (no domain directive)

Change History (8)

comment:1 by Jun Omae, 3 years ago

Notes: 2 plugins on trac-hacks.org are using template named with j*.

$ find . -path '*/templates/j*'
./tracticketstatsplugin/trunk/ticketstats/templates/jsondata.html
./tracticketstatsplugin/tags/2.2.0/ticketstats/templates/jsondata.html
./tracjenkinsplugin/trunk/tracjenkins/templates/jobs.html

comment:2 by Christian Boos, 3 years ago

Sure, the 'j' prefix is just a hack that allowed me to have both series of templates in the same folder while making the conversion. It's not supposed to stay.

comment:3 by Christian Boos, 3 years ago

Quick overview of the Python code churn, as it stands (related to #12130):

$ git diff trunk...jinja2 --stat | grep ".py"
 contrib/jinjachecker.py                            |  433 +++++++
 contrib/jinjastats.py                              |   63 +
 doc/conf.py                                        |    1 +
 sample-plugins/HelloWorld.py                       |    6 +-
 sample-plugins/Timestamp.py                        |    2 +-
 sample-plugins/revision_links.py                   |    3 +-
 sample-plugins/workflow/CodeReview.py              |    3 +-
 sample-plugins/workflow/StatusFixer.py             |    5 +-
 sample-plugins/workflow/VoteOperation.py           |    3 +-
 setup.py                                           |   29 +-
 trac/about.py                                      |    8 +-
 trac/admin/tests/functional.py                     |   20 +-
 trac/admin/web_ui.py                               |    4 +-
 trac/attachment.py                                 |    3 +-
 trac/config.py                                     |    3 +-
 trac/core.py                                       |    2 +-
 trac/db/api.py                                     |    2 +-
 trac/db/mysql_backend.py                           |    3 +-
 trac/db/postgres_backend.py                        |    3 +-
 trac/db/sqlite_backend.py                          |    3 +-
 trac/db/tests/api.py                               |    2 +-
 trac/dist.py                                       |  229 +++-
 trac/env.py                                        |   10 +-
 trac/mimeview/api.py                               |  105 +-
 trac/mimeview/patch.py                             |    6 +-
 trac/mimeview/pygments.py                          |   58 +-
 trac/mimeview/rst.py                               |    4 +-
 trac/mimeview/templates/jprefs_pygments.html       |   72 ++
 trac/mimeview/tests/api.py                         |  113 +-
 trac/mimeview/tests/functional.py                  |    4 +-
 trac/mimeview/tests/patch.py                       |   24 +-
 trac/mimeview/tests/pygments.data                  |   91 ++
 trac/mimeview/tests/pygments.html                  |   57 -
 trac/mimeview/tests/pygments.py                    |   51 +-
 trac/notification/compat.py                        |   20 +-
 trac/notification/mail.py                          |    3 +-
 trac/notification/prefs.py                         |    5 +-
 trac/notification/tests/mail.py                    |    3 +-
 trac/prefs/tests/functional.py                     |    2 +-
 trac/prefs/web_ui.py                               |   24 +-
 trac/resource.py                                   |    2 +-
 trac/search/web_ui.py                              |    9 +-
 trac/tests/compat.py                               |   12 +-
 trac/tests/functional/better_twill.py              |    1 +
 trac/tests/functional/testcases.py                 |    6 +-
 trac/tests/functional/tester.py                    |    2 +-
 trac/tests/resource.py                             |    2 +-
 trac/ticket/api.py                                 |    3 +-
 trac/ticket/batch.py                               |    3 +-
 trac/ticket/default_workflow.py                    |    3 +-
 trac/ticket/notification.py                        |   21 +-
 trac/ticket/query.py                               |   17 +-
 trac/ticket/report.py                              |    3 +-
 trac/ticket/roadmap.py                             |    8 +-
 trac/ticket/tests/conversion.py                    |    8 +-
 trac/ticket/tests/default_workflow.py              |    4 +-
 trac/ticket/tests/functional/admin.py              |   69 +-
 trac/ticket/tests/functional/default_workflow.py   |   40 +-
 trac/ticket/tests/functional/main.py               |  168 +--
 trac/ticket/tests/query.py                         |   87 +-
 trac/ticket/tests/web_ui.py                        |   14 +-
 trac/ticket/web_ui.py                              |   19 +-
 trac/timeline/tests/web_ui.py                      |    6 +-
 trac/timeline/web_ui.py                            |   24 +-
 trac/util/html.py                                  |  905 ++++++++++++--
 trac/util/presentation.py                          |  225 +++-
 trac/util/tests/__init__.py                        |    4 +-
 trac/util/tests/html.py                            |  254 ++--
 trac/util/tests/presentation.py                    |   17 +
 trac/util/text.py                                  |   48 +
 trac/util/translation.py                           |   21 +-
 trac/versioncontrol/admin.py                       |   12 +-
 trac/versioncontrol/diff.py                        |    5 +-
 trac/versioncontrol/tests/functional.py            |   10 +-
 trac/versioncontrol/web_ui/browser.py              |    6 +-
 trac/versioncontrol/web_ui/changeset.py            |   19 +-
 trac/versioncontrol/web_ui/log.py                  |    4 +-
 trac/versioncontrol/web_ui/util.py                 |    3 +-
 trac/web/__init__.py                               |   19 +-
 trac/web/api.py                                    |   35 +-
 trac/web/auth.py                                   |   16 +-
 trac/web/chrome.py                                 |  657 +++++++---
 trac/web/main.py                                   |   36 +-
 trac/web/tests/api.py                              |   19 +-
 trac/web/tests/chrome.py                           |    2 +-
 trac/wiki/api.py                                   |    3 +-
 trac/wiki/formatter.py                             |   43 +-
 trac/wiki/intertrac.py                             |    4 +-
 trac/wiki/interwiki.py                             |    3 +-
 trac/wiki/macros.py                                |    5 +-
 trac/wiki/tests/formatter.py                       |   35 +-
 trac/wiki/tests/functional.py                      |   41 +-
 trac/wiki/tests/macros.py                          |   44 +-
 trac/wiki/web_ui.py                                |   14 +-
 tracopt/ticket/clone.py                            |   61 +-
 tracopt/ticket/commit_updater.py                   |    3 +-
 tracopt/ticket/deleter.py                          |   73 +-
 tracopt/versioncontrol/git/git_fs.py               |    4 +-
 tracopt/versioncontrol/svn/svn_prop.py             |    3 +-
 tracopt/versioncontrol/svn/tests/svn_fs.py         |  148 ++-

comment:4 by Christian Boos, 3 years ago

Owner: set to Christian Boos
Status: newassigned

I've completed the rebase on latest trunk.

Here's a commented break down of the changesets, to ease reviewing. The changesets are listed oldest first.

Note that I started by squashing all the changes, then created smaller commits according to functional units. While I rebased on trunk, I also added some bug fixes.

Here's the initial infrastructure for the switch to Jinja2:

Quite a few utility functions and classes were added, the lowest level is in trac.util.text (Jinja2 being primarily a text based template engine), then trac.util.html and finally trac.util.presentation:

The integration of Jinja2 then continues in the higher-level common layers of Trac, the wiki formatter and the mimeview API:

All the old direct Genshi imports are converted to the new (backward compatible) ones from trac.util.html:

Now the big piece, enabling the Jinja2 template to be loaded, populated with data and generated:

At this point, we can start to work on the templates proper:

Now we start the conversion module by module, with the occasional commit reflecting the changes on trunk since… last year:

So far we have just a refresh of the original Jinja2 branch, with all the back and forth eliminated.

As I did the merge, I worked on finding a way to be able to render indifferently Jinja2 and legacy Genshi templates, without resorting to the "j" prefixing hack that was convenient during the migration period.

This resulted in some quite important API changes, the main trick being that the "old" API is still supported but if used, it is an indication that Genshi templates should be used. For using Jinja2 templates, the newer API should be used. All the details in the following log message of the following commits:

That's it, thanks for having followed so far, and please give it a shot. The integration is really overdue, I've been using the branch successfully for the past 6 months without major issues, and it's time to spread to good news ;-)

I'm particularly interested in feedback on the last two important API changes commits, as it's quite new and I suspect there are still a few improvements possible.

Another area which still needs work is i18n, as I haven't looked closely there since a while. TODO

Finally, Jinja 2.9 is out (or will be out soon), and it comes with a bug fix I've been waiting for all along, so I plan to adapt to that version and make it a minimal requirement. TODO


$ git diff --stat trunk jinja2-trunk-r15341
 ...
 214 files changed, 15353 insertions(+), 1877 deletions(-)

Well, when we'll remove most of the Genshi templates, that will also give some more deletions!

in reply to:  4 comment:5 by Christian Boos, 3 years ago

Replying to Christian Boos:

Finally, Jinja 2.9 is out

It's out and already used for the AppVeyor and Travis builds. I guess the errors in the functional tests seen there must be due to this new version, as I've not tested 2.9 myself yet.

comment:6 by Christian Boos, 3 years ago

Keywords: +testing added

The branch can now be seen in action in testing.

For example, this are the timings for 00faab14 with 30 lines of context:

testing t.e.o
Waiting (TTFB) 9.65 s 27.25 s
Content Download 409.63 ms 598.59 ms

(measured with the network panel in Chrome)

At my first try, I didn't see much speed difference between the two, so I increased the context to 30 lines, hoping the difference would be more noticeable. It was, as I got a MemoryError as a result! Then I realized we have the SpamFilter installed there, and one of its filter uses the ITemplateStreamFilter interface. Once I disabled that tracspamfilter.filters.trapfield.* component, the results were again in line with the expectations (i.e. what you see in the table above).

So I hope that this will be a compelling argument for people to upgrade their plugins and get rid of the ITemplateStreamFilter interface support. As soon as you have one active, you lose the performance improvements gained with the switch to Jinja2. And the hack for supporting the stream filters seems to be quite memory hungry.

by Christian Boos, 3 years ago

make the SpamFilter compatible with the jinja2-trunk-r.. branch after [0f3aee46/cboos.git]

comment:7 by Christian Boos, 3 years ago

API Changes: modified (diff)
Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Branch landed on trunk today [15406-15474].

Note: See TracTickets for help on using tickets.