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

Use JSON.parse instead of literal JS for callback formatting #1370

Merged
merged 19 commits into from
Apr 7, 2021

Conversation

WilliamVenner
Copy link
Contributor

@WilliamVenner WilliamVenner commented Mar 18, 2021

https://github.com/GoogleChromeLabs/json-parse-benchmark

What kind of change does this PR introduce? (check at least one)

  • Other, please describe:

Optimisation

Does this PR introduce a breaking change? (check one)

  • No

The PR fulfills these requirements:

Other information:

Believe it or not, JSON.parse is actually faster than directly evaluating literal JavaScript for JSON objects.

It does actually make sense - when evaluating actual JavaScript the interpreter is looking for variables to interpolate, and is using a parsing pipeline made for interpreting actual code. Whereas with JSON.parse, it's just looking at raw JSON, which is of course much faster than interpreting actual JavaScript.

This should introduce up to x2 speed improvements for formatting callback JS.

Unfortunately I don't know how to set up the dev environment for Tauri right now, it's very complex and I failed the last time I tried. Just want to get this PR up here so it will at least eventually be implemented. If I manage to get a dev environment set up or a maintainer can contribute (edits are on) then the following stuff needs doing for this PR to be complete:

  • Changes file
  • Object syntax checking - only JSON.parse if the JSON string begins with { or [ (the speed improvement only applies for JSON objects/arrays, not literals such as strings, booleans, numbers, etc.)
  • String length check + use literal if larger than the maximum string length

ECMAScript 2016 (ed. 7) established a maximum length of 2^53 - 1 elements. Previously, no maximum length was specified. In Firefox, strings have a maximum length of 2**30 - 2 (~1GB).

@WilliamVenner WilliamVenner requested a review from a team March 18, 2021 02:36
@WilliamVenner WilliamVenner requested a review from a team as a code owner March 29, 2021 17:59
WilliamVenner and others added 2 commits March 29, 2021 19:58
Co-authored-by: Lucas Fernandes Nogueira <lucas@tauri.studio>
@WilliamVenner
Copy link
Contributor Author

Tested the following conditions:

  • Responding with a HashMap
  • Responding with a >1 GB HashMap (took a long time, but did send as a js literal)
  • Responding with a primitive &'static str and String

@WilliamVenner
Copy link
Contributor Author

WilliamVenner commented Apr 6, 2021

/// Safely transforms & escapes a JSON String -> JSON.parse('{json}')
// Single quotes are the fastest string for the JavaScript engine to build.
// Directly transforming the string byte-by-byte is faster than a double String::replace()
pub fn escape_json_parse(mut json: String) -> String {
const BACKSLASH_BYTE: u8 = '\\' as u8;
const SINGLE_QUOTE_BYTE: u8 = '\'' as u8;
// Safety:
//
// Directly mutating the bytes of a String is considered unsafe because you could end
// up inserting invalid UTF-8 into the String.
//
// In this case, we are working with single-byte \ (backslash) and ' (single quotes),
// and only INSERTING a backslash in the position proceeding it, which is safe to do.
//
// Note the debug assertion that checks whether the String is valid UTF-8.
// In the test below this assertion will fail if the emojis in the test strings cause problems.
let bytes: &mut Vec<u8> = unsafe { json.as_mut_vec() };
let mut i = 0;
while i < bytes.len() {
let byte = bytes[i];
if matches!(byte, BACKSLASH_BYTE | SINGLE_QUOTE_BYTE) {
bytes.insert(i, BACKSLASH_BYTE);
i += 1;
}
i += 1;
}
debug_assert!(String::from_utf8(bytes.to_vec()).is_ok());
format!("JSON.parse('{}')", json)
}
#[test]
fn test_escape_json_parse() {
let dangerous_json = String::from(
r#"{"test":"don\\🚀🐱‍👤\\'t forget to escape me!🚀🐱‍👤","te🚀🐱‍👤st2":"don't forget to escape me!","test3":"\\🚀🐱‍👤\\\\'''\\\\🚀🐱‍👤\\\\🚀🐱‍👤\\'''''"}"#,
);
let definitely_escaped_dangerous_json = format!("JSON.parse('{}')", dangerous_json.clone().replace('\\', "\\\\").replace('\'', "\\'"));
let escape_single_quoted_json_test = escape_json_parse(dangerous_json);
let result = r#"JSON.parse('{"test":"don\\\\🚀🐱‍👤\\\\\'t forget to escape me!🚀🐱‍👤","te🚀🐱‍👤st2":"don\'t forget to escape me!","test3":"\\\\🚀🐱‍👤\\\\\\\\\'\'\'\\\\\\\\🚀🐱‍👤\\\\\\\\🚀🐱‍👤\\\\\'\'\'\'\'"}')"#;
assert_eq!(definitely_escaped_dangerous_json, result);
assert_eq!(escape_single_quoted_json_test, result);
}

Copy link
Member

@nklayman nklayman left a comment

Choose a reason for hiding this comment

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

Looks good overall, just a few suggestions. Also, the formatting appears to be off, make sure to reformat the rpc.rs file when you're done.

Copy link
Member

@nklayman nklayman left a comment

Choose a reason for hiding this comment

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

Looks good now, great job!

Copy link
Member

@lucasfernog lucasfernog left a comment

Choose a reason for hiding this comment

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

I love this, such a clever PR!

@nklayman
Copy link
Member

nklayman commented Apr 6, 2021

Delete line 56 to fix the check btw.

@nklayman
Copy link
Member

nklayman commented Apr 7, 2021

Looks like you need to rerun formatting on it :/

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