Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Sep 17, 2020

Conversation

zbraniecki
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

zbraniecki commented Sep 16, 2020

@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
Copy link
Collaborator Author

You can read more about 0.13 plans in #193

@Manishearth
Copy link
Collaborator

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

@zbraniecki zbraniecki merged commit 43f1f59 into projectfluent:master Sep 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants