813862 - Refactor l20n parser for 1.0

Status: RESOLVED FIXED

(L20n :: JS Library, defect, P1)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

Performance tests from performance tests from bug 802857 show that parser is by far the slowest component of L20n stack.

This makes sense, since we never optimized the code for performance and we know for quite a long time that there are a lot of low hanging fruits (like - offset based string scanning vs. chopping lexer).

Main goals:
 - improve performance
 - improve memory cost
 - improve debugging messaging (helpful error messages)
 - enable compiler to show partially compiled complex string (bug 803399)

several other minor bugs can be fixed on the way.

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

The work is being done in parser2 branch of my github clone, and the initial results are extremely promising.

The refactored parser supports 99% of LOL from Gaia's settings app (which is the biggest localization resource in Gaia right now) - the only exception being support for indexes.

On the MBP machine the difference represented in the number of parser of the resource per second is the following:

parser-old: 25 parses/second
parser-new: 5000 parses/second

On Unagi phone the difference is:

parser-old: 2 p/sec
parser-new: 120 p/sec

I expect this performance to stay across resources like this (straight port of .properties file), since all remaining work is in macro, expressions and other factors that are just not going to affect simple entities.

Comment 2

[Staś Małolepszy :stas]

P1 because of the performance impact.

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: parser 2.0

patch from parser2 branch.

It offers major performance improvements (x20 to x100 times), and resolves all dependencies from this bug.

Comment 4

[Staś Małolepszy :stas]

Comment on attachment 686350 [details] [diff] [review]
parser 2.0

Review of attachment 686350 [details] [diff] [review]:
-----------------------------------------------------------------

Outstanding.  r=me with nits, please address them, then squash all your commits, mention this bug in the commit message and merge.

::: lib/parser.js
@@ +1,4 @@
>  (function() {
>    'use strict';
>  
> +  function Parser(emitter) {

The argument change requires changes to l20n.js.  Can you include them in this patch?

@@ +9,5 @@
> +    this.parseString = parseString;
> +    this.addEventListener = addEventListener;
> +    this.removeEventListener = removeEventListener;
> +
> +    this.Error = ParserError;

me likey likey.

@@ +13,5 @@
> +    this.Error = ParserError;
> +
> +    /* Private */
> +
> +    var mSource, mIndex, mLength, mEmitter;

Let's go for _source, _index, _length and _emitter as names for private  variables.

@@ +492,5 @@
>      }
> +
> +    function addEventListener(type, listener) {
> +      if (!mEmitter) {
> +        throw "Emitter not available";

throw Error("...")

@@ +500,4 @@
>  
> +    function removeEventListener(type, listener) {
> +      if (!mEmitter) {
> +        throw "Emitter not available";

throw Error("...")

@@ +899,5 @@
> +        return junk;
> +      }
> +      junk = {
> +        'type': 'JunkEntry',
> +        'content': mSource.slice(mIndex, opening)

Should we try to rewind mIndex here to the end of the last well-formed entity?

@@ +917,5 @@
> +  }
> +  ParserError.prototype = Object.create(Error.prototype);
> +  ParserError.prototype.constructor = ParserError;
> +
> +  this.Parser = Parser;

Replace this with:

  /* Expose the Parser constructor */

  if (typeof exports !== 'undefined') {
    exports.Parser = Parser;
  } else if (this.L20n) {
    this.L20n.Parser = Parser;
  } else {
    this.L20nParser = Parser;
  }

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

The final performance difference. Numbers are operations / second.

On Desktop with IonMonkey:

* Old L20n: 19
* L20n without complex strings: 3020
* L20n with complex strings: 1945

On Desktop without IonMonkey (B2G build):

* Old L20n: 21
* L20n without complex strings: 2117
* L20n with complex strings: 1294

On Unagi phone:

* Old L20n: 2
* L20n without complex strings: 99
* L20n with complex strings: 54

Comment 6

[Staś Małolepszy :stas]

Comment on attachment 686350 [details] [diff] [review]
parser 2.0

Review of attachment 686350 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/parser.js
@@ +100,5 @@
> +    function getArray() {
> +      ++mIndex;
> +      return {
> +        type: 'Array',
> +        content: getItemListSoftTrailComma(getValue, ']')

As discussed with Gandalf and Pike IRL, let's change 'content' to 'data' in all values.

@@ +777,5 @@
> +      if (pos > mIndex) {
> +        var start = mIndex;
> +        mIndex = pos;
> +        return {
> +          type: 'Literal',

Let's call this 'Number'.

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

https://github.com/l20n/l20n.js/commit/cc4b0bcab59a798702445e2f49b589bc19c2b029