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

Track expression dependencies #2113

Merged
merged 35 commits into from Mar 7, 2024
Merged

Track expression dependencies #2113

merged 35 commits into from Mar 7, 2024

Conversation

TimSylvester
Copy link
Collaborator

@TimSylvester TimSylvester commented Feb 14, 2024

When building expression trees, we assign and capture what categories of input they depend on, like feature data and zoom level.

When evaluating whether to update a layer, zoom changes only trigger an update if zoom is used in an expression somewhere within the style.


I discovered that the benchmarking I was doing was thrown off by some local changes lingering in my workspace, so I'm re-evaluating...

deps:

Average frame encoding time: 2.650 ms
Average frame rendering time: 6.487 ms

Average frame encoding time: 2.606 ms
Average frame rendering time: 6.392 ms

Average frame encoding time: 2.579 ms
Average frame rendering time: 6.360 ms

Average frame encoding time: 2.673 ms
Average frame rendering time: 6.564 ms

Average frame encoding time: 2.606 ms
Average frame rendering time: 6.536 ms

Average frame encoding time: 2.592 ms
Average frame rendering time: 6.444 ms

Mean encoding: 2.6177

main:

Average frame encoding time: 2.664 ms
Average frame rendering time: 6.47 ms

Average frame encoding time: 2.711 ms
Average frame rendering time: 6.451 ms

Average frame encoding time: 2.639 ms
Average frame rendering time: 6.372 ms

Average frame encoding time: 2.624 ms
Average frame rendering time: 6.472 ms

Average frame encoding time: 2.715 ms
Average frame rendering time: 6.536 ms

Average frame encoding time: 2.662 ms
Average frame rendering time: 6.427 ms

Mean encoding: 2.6692

Difference: 1.95%

Copy link

github-actions bot commented Feb 16, 2024

Bloaty Results (iOS) 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.1% +12.0Ki  +0.1% +16.0Ki    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results-ios/pr-2113-compared-to-main.txt

Copy link

github-actions bot commented Feb 19, 2024

Bloaty Results 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.7%  +964Ki  +1.0%  +313Ki    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-2113-compared-to-main.txt

Compared to d387090 (legacy)

    FILE SIZE        VM SIZE    
 --------------  -------------- 
   +20% +23.1Mi  +406% +24.3Mi    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-2113-compared-to-legacy.txt

Copy link

codecov bot commented Feb 19, 2024

Codecov Report

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

Project coverage is 59.02%. Comparing base (9614b56) to head (c0d719a).

Files Patch % Lines
src/mbgl/style/expression/compound_expression.cpp 59.13% 7 Missing and 40 partials ⚠️
src/mbgl/style/expression/expression.cpp 39.13% 5 Missing and 9 partials ⚠️
src/mbgl/style/expression/format_expression.cpp 31.57% 3 Missing and 10 partials ⚠️
src/mbgl/style/expression/collator_expression.cpp 18.18% 0 Missing and 9 partials ⚠️
src/mbgl/map/map_impl.cpp 70.00% 0 Missing and 6 partials ⚠️
src/mbgl/style/expression/coercion.cpp 68.75% 1 Missing and 4 partials ⚠️
src/mbgl/style/property_expression.cpp 0.00% 0 Missing and 4 partials ⚠️
include/mbgl/style/property_expression.hpp 62.50% 0 Missing and 3 partials ⚠️
src/mbgl/renderer/render_orchestrator.cpp 50.00% 0 Missing and 2 partials ⚠️
src/mbgl/style/expression/find_zoom_curve.cpp 66.66% 0 Missing and 2 partials ⚠️
... and 8 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2113      +/-   ##
==========================================
+ Coverage   58.38%   59.02%   +0.64%     
==========================================
  Files         575      575              
  Lines       28344    28489     +145     
  Branches    11376    11386      +10     
==========================================
+ Hits        16549    16816     +267     
+ Misses       4165     4032     -133     
- Partials     7630     7641      +11     

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

@TimSylvester TimSylvester marked this pull request as draft February 27, 2024 19:49
Copy link
Collaborator

@louwers louwers left a comment

Choose a reason for hiding this comment

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

Could you add tests?

@TimSylvester TimSylvester marked this pull request as ready for review February 29, 2024 23:22
@TimSylvester
Copy link
Collaborator Author

Could you add tests?

Done. I also expanded some existing test cases, as modifications to some areas with low coverage brought the average way down.

@louwers louwers merged commit 25678a4 into main Mar 7, 2024
31 checks passed
@louwers louwers deleted the expr-deps branch March 7, 2024 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants