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 Resolver to operate on impl Write #199

Merged
merged 2 commits into from
Sep 18, 2020

Conversation

zbraniecki
Copy link
Collaborator

No description provided.

@zbraniecki
Copy link
Collaborator Author

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

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

I'm also removing the quirky DisplayableNode - #122

@zbraniecki
Copy link
Collaborator Author

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

@zbraniecki
Copy link
Collaborator Author

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.

Copy link
Collaborator

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems good

fluent-bundle/src/bundle.rs Outdated Show resolved Hide resolved
@Pike
Copy link
Contributor

Pike commented Sep 18, 2020

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
Copy link
Contributor

Pike commented Sep 18, 2020

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

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.

@zbraniecki zbraniecki merged commit b43d147 into projectfluent:master Sep 18, 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.

3 participants