search

projectfluent / fluent-rs

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

Switch Resolver to operate on impl Write #199

Closed zbraniecki closed 4 years ago

zbraniecki commented 4 years ago

Fixes https://github.com/projectfluent/fluent-rs/issues/121

This is the second major change planned for 0.13. It introduces long awaited writer mode in which the main format_pattern takes any impl Write. I kept the old behavior as format_pattern_to_string -> Cow which allows for fast recovery of simple strings as &str.

The performance is really good, and I like the cleanups that it allowed me to do.

zbraniecki commented 4 years ago
Test old format_pattern format_pattern_to_string
simple 7.9256 us 7.5823 us 7.3676 us
menubar 28.793 us 26.566 us 26.688 us
preferences 116.81 us 79.852 us 114.29 us
unescape 1.9559 us 1.0020 us 1.5760 us

Performance with just this patch is already improved even if to_string mode is used, and the impl Write is faster for anything except the simple benchmark. I'm especially excited for the quite significant improvement on the "preferences" benchmark and I believe this can be further improved later on.

zbraniecki commented 4 years ago

I'm also removing the quirky DisplayableNode - https://github.com/projectfluent/fluent-rs/issues/122

zbraniecki commented 4 years ago

@Manishearth can you skim through and confirm if the big picture looks good?

zbraniecki commented 4 years ago

Actually, one functional change happens to how we react to bomb attack.

Since we're populating a buffer, we can't just bail and return {lol9} identifier. Instead, we start populating with values and when we reach our limit, we just switch Scope::dirty to true and start bailing out. The output is visible in the reference test.

I believe it's a fair tradeoff and I'm comfortable with Rust producing different error-scenario output for when the number of placeables exceeds 100, since it's clearly outside of realm of what regular code should produce, but wanted to flag it explicitly here for any reviewers.

Pike commented 4 years ago

Two comments:

Naming is hard, but I'd expect the signature of format_pattern to be similar across implementations. I'd give the new name to the writer one. Maybe just write_pattern?

Documentation for the new method is TBD still? I know we have problems with docs.rs, but still.

Pike commented 4 years ago

Forgot what I actually did: I mulled over cross-bundle refs and format-to-parts, and I don't think this change makes either scenario harder. Maybe format-to-parts would be easier, but then also DOM would be the one that doesn't want to consume strings. But all of that is hypothetical anyway.

From that big-picture POV, lgtm.

zbraniecki commented 4 years ago

Naming is hard, but I'd expect the signature of format_pattern to be similar across implementations. I'd give the new name to the writer one. Maybe just write_pattern?

I like it! Thank you! I'll update the names tomorrow.

Documentation for the new method is TBD still? I know we have problems with docs.rs, but still.

Yeah, I need to revisit docs before 0.13 release - they fell behind to the point where I want to do an overhaul rather than per-PR-updates before the release.