Refactor l20n parser for 1.0

RESOLVED FIXED in 1.0

Status

defect
P1
normal
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: zbraniecki, Assigned: zbraniecki)

Tracking

unspecified
x86
macOS
Dependency tree / graph

Details

()

Attachments

(1 attachment)

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.
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.
Assignee: nobody → gandalf
Depends on: 814636
P1 because of the performance impact.
Priority: -- → P1
Target Milestone: --- → 1.0
Posted patch parser 2.0Splinter Review
patch from parser2 branch.

It offers major performance improvements (x20 to x100 times), and resolves all dependencies from this bug.
Attachment #686350 - Flags: review?(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;
  }
Attachment #686350 - Flags: review?(stas) → review+
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 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'.
https://github.com/l20n/l20n.js/commit/cc4b0bcab59a798702445e2f49b589bc19c2b029
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.