Delay array/objects initialization in compiler

RESOLVED FIXED

Status

L20n
JS Library
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

Details

(Whiteboard: [gaia?])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Delaying array/object initialization in hot paths gives nice performance wins on jsshell and v8, but it adds cost to node.js. Not sure why.
(Assignee)

Comment 1

5 years ago
Created attachment 813648 [details] [diff] [review]
patch

Impact of this patch:

V8:
parse: 
  mean:   5793.33 (0%)
  stdev:  985.25
  sample: 150
compile: 
  mean:   2360 (+4%)
  stdev:  535.26
  sample: 150
get: 
  mean:   1960 (-37%)
  stdev:  535.21
  sample: 150

JSSHELL:
parse: 
  mean:   5197.37 (+6%)
  stdev:  565.82
  sample: 150
compile: 
  mean:   1327.69 (-32%)
  stdev:  204.28
  sample: 150
get: 
  mean:   1803.17 (-19%)
  stdev:  334.05
  sample: 150

Node.js:
parse: 
  mean:   7532.81 (-7.000000000000001%)
  stdev:  503.86
  sample: 150
compile: 
  mean:   2060.31 (+64%)
  stdev:  120.85
  sample: 150
get: 
  mean:   1264.15 (-4%)
  stdev:  89.19
  sample: 150
ready: 
  mean:   5513.97 (-1%)
  stdev:  343.42
  sample: 150
Attachment #813648 - Flags: feedback?(stas)
(Assignee)

Comment 2

5 years ago
On l20n-master it shaved median from 1108 to 1031.
(Assignee)

Comment 3

5 years ago
One thing to notice is that it means that with this patch we sometimes return null in place of an array/object to the developer.

If we want to make it easier for devs to iterate over objects, we may want to remove this change.
(Assignee)

Comment 4

5 years ago
Created attachment 813678 [details] [diff] [review]
patch

actually, my previous numbers for FxOS are not real, the bindings need to check if attributes are null.

This is an updated patch
I'm running perf tests now.
Attachment #813648 - Attachment is obsolete: true
Attachment #813648 - Flags: feedback?(stas)
Attachment #813678 - Flags: feedback?(stas)
(Assignee)

Comment 5

5 years ago
yeah, the win is smaller than 1031. It's 1093 which is still -17ms.
Comment on attachment 813678 [details] [diff] [review]
patch

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

I like where this is going, but please redo the tests with fixed node.index and attributes: {}.

::: lib/l20n/compiler.js
@@ +227,2 @@
>        var i;
> +      if (node.index) {

I think this test always passes; see the comment further down.

@@ +279,4 @@
>        // accordingly.
>        var entity = {
>          value: this.getString(ctxdata),
> +        attributes: null

Undo?

With attributes: {} we're not changing the API (no changes needes in bindings/) and we initialize the object lazily, only once the entity is requested.

::: lib/l20n/parser.js
@@ +403,4 @@
>  
>        var ch = _source.charAt(_index);
>        var value = getValue(true, ch);
> +      var attrs = null;

You'll need to do the same for the index in the parser (line 452).  Currently, empty indexes are initialized to [] in the AST, which makes the if (node.index) test in the compiler always pass.
Attachment #813678 - Flags: feedback?(stas) → feedback+
Whiteboard: [gaia?]
(Assignee)

Comment 7

5 years ago
(In reply to Staś Małolepszy :stas from comment #6)
> You'll need to do the same for the index in the parser (line 452). 
> Currently, empty indexes are initialized to [] in the AST, which makes the
> if (node.index) test in the compiler always pass.

that's not true. In my patch I change the default index passed to getEntity to null.
(Assignee)

Comment 8

5 years ago
Created attachment 813825 [details] [diff] [review]
patch

updated patch. One more change is to check hash literal in compiler for index
Assignee: nobody → gandalf
Attachment #813678 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #813825 - Flags: review?(stas)
(In reply to Zbigniew Braniecki [:gandalf] from comment #7)

> that's not true. In my patch I change the default index passed to getEntity
> to null.

My apologies, I don't know how I looked at the patch :/
Comment on attachment 813825 [details] [diff] [review]
patch

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

Crazy gains on SpiderMonkey, interesting.  Good work!

r=me with the comment below.

::: lib/l20n/compiler.js
@@ +634,5 @@
>        // if absent, `defaultKey` and `defaultIndex` are undefined
>        var defaultKey;
> +      var defaultIndex = index !== null && index.length ?
> +                           index.shift() :
> +                           undefined;

You can replace this with:

  var defaultIndex = index ? index.shift() : undefined;
Attachment #813825 - Flags: review?(stas) → review+
(Assignee)

Comment 11

5 years ago
https://github.com/l20n/l20n.js/commit/be0c4f6a54af4bad23d292e68dc8afc7738cacd8

Not closing yet, I want to file a bug against JSSHELL first.
(Assignee)

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.