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

Performance decrease after 6.9 due to JSON.parse of already json string #819

Closed
dzirtusss opened this issue Apr 24, 2017 · 2 comments
Closed

Comments

@dzirtusss
Copy link
Contributor

There is some performance decrease for RoR versions after 6.9 which looks like this (from our internal server-side benchmarks):

----------------
 Benchmarking master VS test-ror
----------------
visitor_test1              79.751ms VS 92.823ms, +16.39% BAD !!!
visitor_test2              433.685ms VS 493.804ms, +13.86% BAD !!!
visitor_test3              123.157ms VS 134.358ms, +9.09% BAD !!!
user_test1                 131.78ms VS 144.51ms, +9.66% BAD !!!
user_test2                 1038.557ms VS 1128.468ms, +8.65% BAD !!!
user_test3                 162.683ms VS 174.69ms, +7.38% BAD !!!

The reason for it is JSON.parsing of the already json string to hash (and later back), which should be generally just a copy-paste operation

props.is_a?(String) ? JSON.parse(ERB::Util.json_escape(props)) : props

https://github.com/shakacode/react_on_rails/blob/master/lib/react_on_rails/react_component/options.rb#L17

justin808 referenced this issue Apr 24, 2017
…erformance

Initial implementation started by https://github.com/eliaslopezgt and then https://github.com/squadette
* Refactor console output to use <script> tag with a certain ID
* Use <script type="application/json"> with JSON content instead of attribute
* Pretty-print JSON in development and testing
* getAttribute() is better, because it handles attributes which are not present.
* Return skip display none to react on rails configuration
* Extract data-js-react-on-rails-store attribute to the const
* Return to_json method call to app_props_server_render to ensure handling the both cases
* Delete unnecessary style method and HIDDEN constant from react component module
* Improve jsonEl parsing to handle props as already converted from JSON to a string
* Move handling props already converted to json string to ruby code
* Escape json from props as json string
* Add test to ensure that even json string props are sanitezed
* Delete test json props because we parse all props to a hash now
@justin808
Copy link
Member

Reverting the line here:

7ccd603#diff-ab00ff0a0648717a06431522f7eb6861R16

@justin808
Copy link
Member

Fixed by #821 and shipped in 7.0.0.

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

No branches or pull requests

2 participants