search

projectfluent / fluent-rs

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

Switch AST to be generic (either String or &str) #198

Closed zbraniecki closed 3 years ago

zbraniecki commented 3 years ago

This patchset is intended for merging.

It refactors the AST to be generic over Slice which initially is just String or &str.

The resolver, for now, is just operating over &str one, but in theory we could make it Borrow<ast::Resource<&str>> for ast::Resource<String>.

Performance

Performance looks good:

fluent-syntax

Gnuplot not found, disabling plotting
parse/"simple"          time:   [36.829 us 37.023 us 37.257 us]
                        change: [-7.2331% -6.3743% -5.4848%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 11 outliers among 100 measurements (11.00%)
  1 (1.00%) low mild
  4 (4.00%) high mild
  6 (6.00%) high severe
parse/"preferences"     time:   [405.68 us 407.36 us 409.35 us]
                        change: [-13.570% -12.578% -11.495%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
  2 (2.00%) high mild
  4 (4.00%) high severe
parse/"menubar"         time:   [111.84 us 112.23 us 112.61 us]
                        change: [-11.467% -10.780% -10.055%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
  2 (2.00%) high mild
  2 (2.00%) high severe

unicode                 time:   [7.7904 us 7.8246 us 7.8611 us]
                        change: [+4.4319% +5.3643% +6.3307%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 5 outliers among 100 measurements (5.00%)
  3 (3.00%) high mild
  2 (2.00%) high severe

parse_ctx/"browser"     time:   [447.54 us 449.36 us 451.25 us]
                        change: [-12.656% -11.967% -11.285%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
  3 (3.00%) high mild
Benchmarking parse_ctx/"preferences": Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 5.2s or reduce sample count to 60
parse_ctx/"preferences" time:   [1.0211 ms 1.0252 ms 1.0296 ms]
                        change: [-13.658% -12.773% -11.862%] (p = 0.00 < 0.05)
                        Performance has improved.

fluent-bundle

Not much impact here, as expected. We'll get more out of further refactors:

Gnuplot not found, disabling plotting
resolve/"simple"        time:   [7.1773 us 7.2032 us 7.2302 us]
                        change: [-5.7617% -5.0180% -4.3403%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
  2 (2.00%) high mild
  3 (3.00%) high severe
resolve/"preferences"   time:   [110.33 us 111.79 us 113.52 us]
                        change: [-5.4094% -4.1042% -2.7613%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
  3 (3.00%) high mild
  2 (2.00%) high severe
resolve/"menubar"       time:   [27.435 us 27.643 us 27.893 us]
                        change: [+2.7149% +3.9327% +5.2268%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 2 outliers among 100 measurements (2.00%)
  1 (1.00%) high mild
  1 (1.00%) high severe
resolve/"unescape"      time:   [1.8450 us 1.8561 us 1.8691 us]
                        change: [-1.9740% -1.1584% -0.3769%] (p = 0.01 < 0.05)
                        Change within noise threshold.
Found 10 outliers among 100 measurements (10.00%)
  1 (1.00%) low mild
  5 (5.00%) high mild
  4 (4.00%) high severe
zbraniecki commented 3 years ago

This patchset also quite significantly simplifies the serde interactions. I simplified the AST a bit to make it easier to interoperate with the reference AST, and pulled serde derive into the AST itself, minus the CommentDef which is a small hack to handle single-string comments in reference AST.

zbraniecki commented 3 years ago

@stasm @Manishearth could you take a look at this? There aren't many significant changes, so mostly am curious about your take on the AST changes and the Slice.

I commented out a single reference test - clrf, because I need a bit more work to do to handle comparing a Pattern with multiple TextElement elements vs one with merged, because the parser produces vec![TextElement("Foo"), TextElement("\n"), TextElement("second line")] while the reference JSON is vec![TextElement("Foo\n"), TextElement("second line")]. I'd like to add custom PartialEq between two Patterns which would concatenate all consequitive TextElement elements before comparing, but Rust doesn't support specialization yet, so I can't easily put PartialEq<ast::Pattern<S: Slice>> and have ast:Pattern derive default PartialEq for generic S. So I comment it out for now, but plan to revisit later, and verified that everything else matches.

zbraniecki commented 3 years ago

You can read more about 0.13 plans in https://github.com/projectfluent/fluent-rs/issues/193

Manishearth commented 3 years ago

I like this! I haven't looked closely at all the changes but it seems pretty straightforward.