-
Notifications
You must be signed in to change notification settings - Fork 28
Compiling MessageContext #71
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
Compiling MessageContext #71
Conversation
and other bug fixes
…mbers Thanks Babel!
dedent_ftl was converting unicode strings to bytestrings
Thanks for sharing this. I see that you mention profiling on your README, can you share what's showing up in the profiles for you? Asking 'cause I've done so in my tooling environment, and I found the Fluent parser to be pretty bad, compared to other l10n fileformats we check for Firefox. My take from the profile I had was that this is mostly because the python interpreter reacts to the code differently than the js engines (which JIT). I personally expect that we'll want to have a python parser which is written to perform well on python in the not-so-distant future. On the actual change here, I share the concerns about security that you added to the README, and I think we'll need strong answers. |
@Pike So all of my benchmarks so far have focussed on the runtime performance of the compiled code. In many cases, the code is extremely minimal (e.g. returning a constant for the simple case - https://github.com/projectfluent/python-fluent/pull/71/files#diff-8ea7f46014c70a54a5d42179849a3a5aR56 ). For more complex cases, it is dominated by things like number formatting, or the plural rules lookup (which uses Babel's implementation and can be quite expensive for complicated rules). The benchmark script I've added could be extended to do benchmarks of the parser though. |
@spookylukey - is your patchset ready for review at this point? |
@zbraniecki - it builds on top of #67, which is not yet merged or reviewed, otherwise I think it is ready, but it might make more sense to wait for #67, or at least compare with the final commit on that branch instead of comparing with current master. I've also been working on elm-fluent over the past few weeks, which is an FTL-to-Elm compiler implemented in Python, and so shares a large amount of structure with this compiler. Some of the work I've done on that has made me think of clean ups and improvements I could do for this branch, but they can wait. |
Thanks! I take it that #67 is ready for review then? |
@zbraniecki Yes, it's ready, I fixed the last issue with the test suite failing on Python 2. |
Thank you! We'll coordinate and try to get it on the review schedule as soon as possible! |
Motivation: 1. Simplicity - makes it easier for us to type the return values of these functions, because they no longer return tuples. 2. Performance - less tuple packing/unpacking. This will probably only make a difference when messages call other messages, because we still have to pack the tuple for the outer MessageContext.format call.
Specifically, multi-assignments using tuple unpacking syntax
in line with projectfluent#67 (comment)
This PR is obsolete, my latest branch is https://github.com/django-ftl/python-fluent/tree/compiler_implementation which has many changes from this, but I will start a new PR for that later. |
This is a second implementation of
MessageContext
using a compile-to-python strategy. The PR builds on top of #67 and should be reviewed after that has been merged.My idea is that both implementations will be available, with the compiler the default because it is much faster (some benchmarks included in the new benchmark script). The interpreter is much easier to add new things to (which could be important for other contributors) and has some features/behaviour that the compiler doesn't have (documented). In addition, the two implementations can help test each other to some extent, and if you have any planned extensions, having two implementations, although twice the work, can help to ensure that the new feature doesn't limit you to a specific implementation strategy.
For example, I have an escapers feature that I need for django-ftl - see docs , which I've been able to implement for both the interpreter and compiler, but starting with the interpreter was easier. (That branch hasn't been updated for the 0.6 spec changes yet).
This branch is mostly complete, but probably needs some final cleanup. I'm opening it now in order to make @stasm aware of its existence (especially as I'm away for a bit now). It doesn't change that much from the interpreter
MessageContext
already implemented, but does clean up and clarify a few things (e.g. the error handling strategy), andMessageContext
gains acheck_message
method (currently undocumented).If you want to look at it, I imagine that starting with reading the
tests/test_compiler.py
tests will be the best way to go - it will illustrate the whole strategy, and the kind of optimizations that are implemented, which should explain a lot of thecompiler.py
andcodegen.py
code.