Add FluentMessage #366

stasm · 2019-04-29T16:08:36Z

Fix #365. Obsoletes #322 and #360 (on which it's based).

zbraniecki · 2019-04-30T17:12:06Z

fluent/src/bundle.js

-   */
-  get messages() {
-    return this._messages[Symbol.iterator]();
+    this._dirty = new WeakSet();


what's that? It doesn't seem to be used anywhere

It's used in createScope, and then in Pattern to detect cyclic references.

zbraniecki · 2019-04-30T17:13:49Z

fluent/src/bundle.js

+  format(id, args, errors = []) {
+    if (!this._messages.has(id)) {
+      errors.push(`Message not found: "${id}"`);
+      return undefined;


Why undefined rather than null? Is the user expected to always check if the message exists before trying to format it? I thought format("non-existing") -> null is a reasonable API use and not an error?

I decided to used undefined here and when formatting missing attributes in order to tell missing values from null values apart. Consider:

foo = .attr = Attr

format("foo") → null,

format("bar") → undefined.

Do you think they should both return null instead?

That's an interesting question. I'm torn. For me the question boils down to - should attempting to format a message that doesn't existing in the bundle be an error (which means, you should always call hasMessage before you call format)?

I think the bindings should always check, yes. They should also use formatValue and formatAttribute, rather than format. format is intended for more imperative approaches. The fact that it returns undefined on missing messages was intended to be consistent with formatAttribute returning undefined on missing attributes.

It also works well semantically, I think. The difference between null and undefined maps nicely to Fluent's notions of value-less messages vs. missing messages. Attempting to format a missing message should result in an error being reported. As a developer, there's little incentive for me to intentionally format missing messages :) So it's likely an error scenario, which I'd like to be able to react to.

zbraniecki · 2019-04-30T17:15:47Z

fluent/src/bundle.js

+      bundle: this,
+      dirty: this._dirty,
+      // TermReferences are resolved in a new scope.
+      insideTermReference: false,


I struggled with that in Rust because of lifetimes. While the exact problem is very rust specific and doesn't warrant design changes, I kinda like what I ended up with - locale_args in the scope - https://github.com/projectfluent/fluent-rs/blob/master/fluent-bundle/src/resolve.rs#L244

Maybe worth considering here? Not a blocker of course.

Nice! So local_args is both a store of arguments passed to the term and a flag meaning we're resolving a term now? I like it.

zbraniecki · 2019-04-30T17:17:05Z

fluent/src/resource.js

@@ -55,7 +56,7 @@ const MAX_PLACEABLES = 100;
 /**
 * Fluent Resource is a structure storing a map of parsed localization entries.
 */
-export default class FluentResource extends Map {
+export default class FluentResource extends Set {


Beyond updating the comment just a line above, can we consider Array vs. Set here? We won't likely be looking for duplicate-free set, so why not Array?

Good idea, let me test it with make perf.

Sweet, extending Array gives a slight speed-up in addResource. I guess iterating over the elements of an array is faster than of a set. Thanks for the suggestion!

well, Set does more - it has to hash and look for duplicates on insertion.

I know, but interestingly, there's no noticeable difference on the benchmark which inserts. Instead, it's the one which iterates over the elements which sped up :)

stasm · 2019-04-30T18:05:26Z

Here's the output of make perf compared to master:

parse-runtime/simple:
  mean:   2.46 (+3%)
  stdev:  0.49
  sample: 30
create-runtime/simple:
  mean:   0.44 (-54%)
  stdev:  0.12
  sample: 30
resolve-runtime/simple:
  mean:   1.15 (+35%)
  stdev:  0.53
  sample: 30
parse-runtime/preferences:
  mean:   8.36 (-1%)
  stdev:  1.33
  sample: 30
create-runtime/preferences:
  mean:   0.74 (-43%)
  stdev:  0.15
  sample: 30
resolve-runtime/preferences:
  mean:   9.1 (+11%)
  stdev:  1.57
  sample: 30
parse-runtime/menubar:
  mean:   4.95 (+13%)
  stdev:  0.66
  sample: 30
create-runtime/menubar:
  mean:   0.53 (-46%)
  stdev:  0.17
  sample: 30
resolve-runtime/menubar:
  mean:   2.36 (+15%)
  stdev:  0.51
  sample: 30
parse-runtime/workload-low:
  mean:   4.99 (-5%)
  stdev:  0.78
  sample: 30
create-runtime/workload-low:
  mean:   0.46 (-57%)
  stdev:  0.06
  sample: 30
resolve-runtime/workload-low:
  mean:   1.82 (+10%)
  stdev:  0.34
  sample: 30

I think create-runtime (a benchmark which I added when working on this PR; it tests the bundle creation and addResource) is faster mostly because FluentResources aren't subclasses of Map anymore. I was expecting parsing to go up a bit because it now creates FluentMessage instances inside of the FluentResource, but there aren't any significant changes.

We're paying more for resolving and I think it's the price of the encapsulation: bundle.formatValue calls createScope, then message.resolveValue, which calls Type. Only then do we check if the value is a simple string and return it. It happens later than on master which—because no encapsulation is enforced—checks the type of the message right away inside of bundle.format. However, it's still not bad (up to +35% on simple.ftl) and combined with the speed-ups in addResource, a one-time create-parse-format cycle is generally faster, or at least the same, in this PR and on master. For the aforementioned simple.ftl, it takes 4.18 ms on my laptop on master, and 4.05 ms with this PR.

zbraniecki · 2019-04-30T18:10:16Z

It would be nice to have a create-parse-format cycle perf end-to-end test :)

stasm · 2019-07-11T11:14:18Z

We're going to the go with the formatPattern proposal in #380.

stasm and others added 25 commits April 26, 2019 13:23

create-runtime

58dbb91

Initial format/compound changes by @unclenachoduh

6a67193

Factor out factoring functions; document public API

2183734

Use bundle.compound in tools/

e4ecf3b

Return undefined if message/attribute not found

5ec39aa

Remove the FluentBundle.messages iterator

f4f4f7d

Create FluentMessages in addResource

c16deaf

Parse messages to {*, ...attrs} objects

65c762a

Fix fallback in {Message,Term}Reference

4a6ec1d

FluentMessage._value, _attributes

fe7336d

Create bundle-less FluentMessages during parsing

47d46be

FluentBundle.format{Value,Attribute}

9e55a28

FluentResource extends Set

64d1e12

Handle FluentNone in format methods

d357c48

Create the dirty WeakMap once

cdd9692

Use local scope for resolving terms

868f4d5

Fix lint errors

8b29dc2

Regenerate runtime parser fixtures

83d08b6

Report unknown attrs once

4ef2ed4

Update tests

a7871c9

Handle null values in bundle.format

4bc6ba8

FluentMessage.hasValue

11da57d

Regenerate the fixtures

3164e33

Document FluentMessage

d6fe969

Document format methods

b22f04d

stasm mentioned this pull request Apr 29, 2019

Store messages as FluentMessage in the runtime #365

Closed

stasm requested review from Pike and zbraniecki April 30, 2019 17:08

zbraniecki reviewed Apr 30, 2019

View reviewed changes

FluentResource extends Array

b946b74

stasm closed this Jul 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FluentMessage #366

Add FluentMessage #366

stasm commented Apr 29, 2019

zbraniecki Apr 30, 2019

stasm Apr 30, 2019

zbraniecki Apr 30, 2019

stasm Apr 30, 2019

zbraniecki Apr 30, 2019

stasm Apr 30, 2019

zbraniecki Apr 30, 2019

stasm Apr 30, 2019 •

edited

zbraniecki Apr 30, 2019

stasm Apr 30, 2019

stasm Apr 30, 2019 •

edited

zbraniecki Apr 30, 2019

stasm Apr 30, 2019

stasm commented Apr 30, 2019 •

edited

zbraniecki commented Apr 30, 2019

stasm commented Jul 11, 2019

Add FluentMessage #366

Add FluentMessage #366

Conversation

stasm commented Apr 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stasm Apr 30, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stasm Apr 30, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stasm commented Apr 30, 2019 • edited

zbraniecki commented Apr 30, 2019

stasm commented Jul 11, 2019

stasm Apr 30, 2019 •

edited

stasm Apr 30, 2019 •

edited

stasm commented Apr 30, 2019 •

edited