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

Speed-up HTML escaping a little bit #100

Merged
merged 1 commit into from
Jul 28, 2024

Conversation

Kijewski
Copy link
Collaborator

$ cargo bench --bench escape

Before the PR: [3.6464 µs 3.6512 µs 3.6564 µs]
With the PR:   [3.3633 µs 3.3753 µs 3.3887 µs]

Around 7% faster.

const ESCAPED_BUF_LEN: usize = b"&#__;".len();

#[test]
fn simple() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please pick a better name. Not very helpful if it ever fails. 😉

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If "simple" is broken, then "simply everything" is broken? :D Changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Try explaining that to future you. 😛

}

const MIN_CHAR: u8 = b'"';
const MAX_CHAR: u8 = b'>';
/// If the character needs HTML escaping, then return the decimal representation of the codepoint.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// If the character needs HTML escaping, then return the decimal representation of the codepoint.
/// Returns the decimal representation of the codepoint if the character needs HTML escaping.

}

/// For characters that need HTML escaping, the codepoint formatted as decimal digits,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a word is missing here:

Suggested change
/// For characters that need HTML escaping, the codepoint formatted as decimal digits,
/// For characters that need HTML escaping, the codepoint is formatted as decimal digits,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Stupid question: How can I add a "suggested change" when I review code?

Copy link
Contributor

Choose a reason for hiding this comment

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

Welcome to "github trick of the day" :D

Like this:

```suggestion
yo
```

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahhh, thank you! :D

```text
$ cargo bench --bench escape

Before the PR: [3.6464 µs 3.6512 µs 3.6564 µs]
With the PR:   [3.3633 µs 3.3753 µs 3.3887 µs]
```

Around 7% faster.
@Kijewski
Copy link
Collaborator Author

Oh, I just noticed that we are at issue #100 already! 🥳

@GuillaumeGomez GuillaumeGomez merged commit d39ee22 into rinja-rs:master Jul 28, 2024
17 checks passed
@GuillaumeGomez
Copy link
Contributor

We're doing great together. Let's hope it'll continue for a long time. 🎉

@Kijewski Kijewski deleted the pr-no-jetski branch July 28, 2024 21:28
@Kijewski
Copy link
Collaborator Author

I hope so, too! :)

@GuillaumeGomez
Copy link
Contributor

❤️

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