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

Enable cache of XML fragment for node, way and relation #3813

Closed
wants to merge 1 commit into from

Conversation

yuiseki
Copy link

@yuiseki yuiseki commented Nov 20, 2022

What

This pull request changes just three files on this rails application

  • app/views/api/nodes/_node.xml.builder
  • app/views/api/ways/_way.xml.builder
  • app/views/api/relations/_relation.xml.builder

Why

  • Currently, this rails app does not cache fragment of XML
  • This should slow down the XML building
  • Whether the caching mechanism is memory or files, it should be faster than executing SQL queries and building XML
  • These files appear to have been last edited four years ago, but modern Rails' caching of XML fragments has progressed

How

  • Change the Rails Model instance handled by *.xml.builder to be passed to cache method of the Rails view
  • If there are no changes to the records in the Rails Model, the SQL query execution and XML construction will be skipped and the cache will be used
  • If the Rails Model record changes, the cache is destroyed and the XML is updated appropriately

My opinion

  • This change should be carefully considered as it will likely affect the behavior of the Production environment
  • Still, I believe it should have a big positive impact on performance
  • Please consider it and let me know what you think!

@tomhughes
Copy link
Member

Well this will have no effect on our production environment because we don't serve those endpoints from the rails code - they are served by cgimap.

If we were using it in production then the problem would be the impact on our cache and the fact that unless we provisioned a lot more cache it would probably cause more important things (like user sessions) to be evicted.

@mmd-osm
Copy link
Contributor

mmd-osm commented Dec 3, 2022

OpenHistoricalMap uses the rails code, and I’m not aware of anyone reporting issues with iD. OP also didn’t present any runtime comparison.

I would close this issue.

@yuiseki yuiseki closed this Dec 4, 2022
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.

None yet

3 participants