Skip to content

#7587 - $.parseJSON huge performance optimization by skipping regex #264

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

Closed
wants to merge 3 commits into from

Conversation

fracmak
Copy link

@fracmak fracmak commented Mar 7, 2011

this checking skips the expensive regex/replace validation on the JSON when using the browser's native JSON parser because the threat to security is gone. My tests show this reduces the runtime of the parseJSON function to 1/3rd of the original code.

http://bugs.jquery.com/ticket/7587

Performance test here: http://jsperf.com/parsejson-optimization/2

…egex in favor of try/catching the error returned by the JSON parser
@jboesch
Copy link
Contributor

jboesch commented Mar 8, 2011

Those perf tests are sexy :)
I got burnt on this, but you'll need to change your spacing to use tabs instead of spaces (the if/else code blocks aren't aligned):
https://github.com/fracmak/jquery/blob/7f8623de44978be5ddcf9667741592d3520a7551/src/core.js#L521

@fracmak
Copy link
Author

fracmak commented Mar 8, 2011

oops, sorry about that, fixed!

@rwaldron
Copy link
Member

Please check the whitespace. http://docs.jquery.com/JQuery_Core_Style_Guidelines

@fracmak
Copy link
Author

fracmak commented Mar 14, 2011

I think I fixed all the whitespace issues, let me know if I missed any.

@danheberden
Copy link
Member

Simplified the process a bit in #300 - any other comments/feedback on this feature should be made there.

Thanks for getting the ball rolling and perf tests up fracmak!

@danheberden danheberden closed this Apr 5, 2011
@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants