projectfluent / fluent-rs

Rust implementation of Project Fluent
https://projectfluent.org
Apache License 2.0
1.09k stars 97 forks source link

Refactor resolver errors to provide better fallbacking and return errors out of formatting #93

Closed zbraniecki closed 5 years ago

zbraniecki commented 5 years ago

This is a pretty major refactor of the Resolver, in a pretty POC state of work.

It aims to achieve three main goals:

The code is far from being completed, but it already renders really promising performance wins:

master:

Benchmarking resolve/"simple": Collecting 100 samples in estimated 5.0014                                                                         resolve/"simple"
                        time:   [423.91 ns 427.41 ns 431.49 ns]
                        change: [+307.52% +312.14% +317.44%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 3 outliers among 100 measurements (3.00%)
  2 (2.00%) high mild
  1 (1.00%) high severe
Benchmarking resolve/"menubar": Collecting 100 samples in estimated 5.007                                                                         resolve/"menubar"
                        time:   [98.715 us 99.508 us 100.49 us]
                        change: [+188.03% +193.45% +199.59%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 3 outliers among 100 measurements (3.00%)
  1 (1.00%) high mild
  2 (2.00%) high severe
Benchmarking resolve/"unescape": Collecting 100 samples in estimated 5.00                                                                         resolve/"unescape"
                        time:   [2.0391 us 2.0608 us 2.0865 us]
                        change: [+174.65% +179.37% +183.55%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 2 outliers among 100 measurements (2.00%)
  2 (2.00%) high mild

bundle-tests branch:

Benchmarking resolve/"simple": Collecting 100 samples in estimated 5.0002                                                                         resolve/"simple"
                        time:   [99.838 ns 100.40 ns 101.05 ns]
                        change: [-76.878% -76.609% -76.357%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high mild
Benchmarking resolve/"menubar": Collecting 100 samples in estimated 5.135                                                                         resolve/"menubar"
                        time:   [33.873 us 34.129 us 34.424 us]
                        change: [-66.651% -66.045% -65.508%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high mild
Benchmarking resolve/"unescape": Collecting 100 samples in estimated 5.00                                                                         resolve/"unescape"
                        time:   [735.93 ns 740.36 ns 745.43 ns]
                        change: [-65.005% -64.488% -63.949%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
  3 (3.00%) high mild

All three benchmarks put the branch at 60-70% faster, mostly due to lack of string copying for simple strings.

I'd like to consider this refactor for 0.6.

@stasm - this is likely too early for review, but the POC is complete enough for you to asses the general approach. Lmk!

Pike commented 5 years ago

Out of curiosity, have you considered the pre-evaluated algorithm we're using in the python resolver now?

The other thing we did in python was to move from dispatch to transformer pattern. That was merely because dispatch was slow. Is that a problem in rust?

Also, can we refer to the js resolver as the js resolver? We don't have a reference resolver yet ;-)

zbraniecki commented 5 years ago

This now passes all tests, retains all functionality, and maintains the performance win of ~60% in resolver tests. I still want to:

but the code matures to a full PR now (so that I start getting CI results and coverage results on this branch) :)

zbraniecki commented 5 years ago

@stasm - I'm still working on porting resolver tests and improving coverage, but I believe this code is ready for preliminary review.

If you prefer me to first document it, I'll postpone the review, but it may be easier for me to know if you approve the general direction before I dive into details.

zbraniecki commented 5 years ago

I think that two interesting concepts that I introduced here, which may be useful to port back to JS are:

zbraniecki commented 5 years ago

@manishearth - I'd love to get your feedback on this PR. Majority of the real action is in src/resolve.rs

stasm commented 5 years ago

If you prefer me to first document it, I'll postpone the review, but it may be easier for me to know if you approve the general direction before I dive into details.

Thanks for letting me know. I'll be away next week, so it might best to postpone the review until I'm back.

zbraniecki commented 5 years ago

Okay! I'll work on porting more tests, inline docs and getting review from Manish first then :)

zbraniecki commented 5 years ago

Gonna merge it, and continue working in separate issues.