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

Move expensive WGS84_XY constant setup into ElevationModule #5786

Merged
merged 5 commits into from
Apr 15, 2024

Conversation

leonardehrenfried
Copy link
Member

@leonardehrenfried leonardehrenfried commented Apr 4, 2024

Summary

When profiling something else, I noticed during test setup GeometryUtils takes up to 500ms to initialize. That is because it does quite an expensive lookup of a coordinate reference system in a HSQL embedded database.

Screenshot from 2024-04-04 16-45-42
Screenshot from 2024-04-04 16-45-25

Why is this bad?

I strive to write tests that execute quite quickly so having 500ms startup time for a test is a reason for me to investigate. If I move the initialization into ElevationModule investigating slow tests will be less noisy.

@leonardehrenfried leonardehrenfried requested a review from a team as a code owner April 4, 2024 15:54
@leonardehrenfried leonardehrenfried changed the title Move expensive WGS84_XY constant setup into ElevationModule Move expensive WGS84_XY constant setup into ElevationModule Apr 4, 2024
Copy link

codecov bot commented Apr 4, 2024

Codecov Report

Attention: Patch coverage is 57.14286% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 67.81%. Comparing base (978424c) to head (3c1bb2d).
Report is 13 commits behind head on dev-2.x.

❗ Current head 3c1bb2d differs from pull request most recent head 70bb4ee. Consider uploading reports for the commit 70bb4ee to get more accurate results

Files Patch % Lines
...nner/graph_builder/module/ned/ElevationModule.java 57.14% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             dev-2.x    #5786   +/-   ##
==========================================
  Coverage      67.81%   67.81%           
- Complexity     16531    16533    +2     
==========================================
  Files           1906     1906           
  Lines          72277    72276    -1     
  Branches        7443     7443           
==========================================
  Hits           49016    49016           
  Misses         20740    20740           
+ Partials        2521     2520    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@abyrd abyrd left a comment

Choose a reason for hiding this comment

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

Code changes look good. I proposed a change to the comments:

  • The field is private so it's no longer "shared".
  • My understanding is that looking up the CRS is not particularly slow, it's the database initialization that's slow, and incurred repeatedly when unrelated tests are run.
  • I also combined the two comments into one as a matter of style.
  • Removed the link to the PR in favor of summarizing the reasoning directly in the comment. It's a judgement call in each case, but in this case the motivation outlined in the PR can be expressed in about the same space as a link, and it is generally possible to trace a commit back to the PR in the unlikely case that further detail is needed.

@leonardehrenfried leonardehrenfried requested a review from abyrd April 9, 2024 13:57
@leonardehrenfried leonardehrenfried merged commit b176aee into opentripplanner:dev-2.x Apr 15, 2024
5 checks passed
@leonardehrenfried leonardehrenfried deleted the move-crs branch April 15, 2024 11:18
@t2gran t2gran added this to the 2.6 (next release) milestone May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants