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 Shift(s): 9192442a #5182

Closed
github-actions bot opened this issue Feb 28, 2023 · 7 comments · Fixed by #5242
Closed

Performance Shift(s): 9192442a #5182

github-actions bot opened this issue Feb 28, 2023 · 7 comments · Fixed by #5242
Assignees
Labels
Bot A bot generated issue/pull-request Type: Performance

Comments

@github-actions
Copy link
Contributor

Benchmark comparison has identified performance shifts at

Please review the report below and take corrective/congratulatory action as appropriate 🙂

Performance shift report
       before           after         ratio
     [21b8a2cd]       [9192442a]
     <main~1>         <main>    
+      44.3±0.7ms       55.9±0.9ms     1.26  experimental.ugrid.Mesh.time_eq(6)
+      10.1±0.4ms       13.6±0.3ms     1.34  experimental.ugrid.MeshCoordLazy.time_bounds(10000)
+      8.80±0.2ms       11.7±0.2ms     1.33  experimental.ugrid.MeshCoordLazy.time_bounds(6)
+      3.63±0.2ms       5.39±0.1ms     1.48  experimental.ugrid.MeshCoordLazy.time_points(10000)
+      7.36±0.2ms       10.8±0.2ms     1.46  experimental.ugrid.MeshCoordLazy.time_points(1000000)
+      3.56±0.1ms      4.92±0.08ms     1.38  experimental.ugrid.MeshCoordLazy.time_points(6)
+      44.1±0.5ms         55.3±1ms     1.26  experimental.ugrid.MeshLazy.time_eq(6)
+        39.3±1ms         64.9±2ms     1.65  load.LoadAndRealise.time_load((1280, 960, 5), False, 'NetCDF')
+      39.1±0.9ms       52.2±0.6ms     1.33  load.LoadAndRealise.time_load((1280, 960, 5), True, 'NetCDF')
+        35.7±2ms         49.7±1ms     1.39  load.LoadAndRealise.time_load((2, 2, 1000), False, 'NetCDF')
+        36.9±1ms         50.7±1ms     1.38  load.LoadAndRealise.time_load((2, 2, 1000), True, 'NetCDF')
+      36.8±0.6ms         50.3±1ms     1.37  load.LoadAndRealise.time_load((2, 2, 2), False, 'NetCDF')
+      37.0±0.9ms       49.7±0.9ms     1.34  load.LoadAndRealise.time_load((2, 2, 2), True, 'NetCDF')
+     3.58±0.04ms       6.09±0.2ms     1.70  load.LoadAndRealise.time_realise((2, 2, 1000), False, 'NetCDF')
+     3.68±0.06ms       6.25±0.2ms     1.70  load.LoadAndRealise.time_realise((2, 2, 1000), True, 'NetCDF')
+     3.53±0.08ms       6.05±0.2ms     1.72  load.LoadAndRealise.time_realise((2, 2, 2), False, 'NetCDF')
+     3.61±0.03ms      6.12±0.09ms     1.70  load.LoadAndRealise.time_realise((2, 2, 2), True, 'NetCDF')
+      34.7±0.7ms       42.1±0.8ms     1.22  load.TimeConstraint.time_time_constraint(3, 'NetCDF')
+      18.1±0.7ms       23.0±0.8ms     1.27  load.ugrid.BasicLoading.time_load_mesh(200000)
+      39.0±0.7ms       47.5±0.6ms     1.22  load.ugrid.BasicLoadingTime.time_load_file(200000)
+      15.3±0.4ms       22.0±0.4ms     1.44  load.ugrid.BasicLoadingTime.time_load_mesh(200000)
+      4.46±0.2ms       6.16±0.2ms     1.38  load.ugrid.DataRealisation.time_realise_data(1)
+      7.18±0.3ms       9.24±0.3ms     1.29  load.ugrid.DataRealisation.time_realise_data(200000)
+      4.50±0.3ms       6.04±0.2ms     1.34  load.ugrid.DataRealisationTime.time_realise_data(1)
+      66.1±0.8ms       86.1±0.7ms     1.30  save.NetcdfSave.time_netcdf_save_cube(1, True)
+      49.2±0.3ms         69.6±1ms     1.41  save.NetcdfSave.time_netcdf_save_mesh(1, True)

Generated by GHA run 4287641110

@github-actions github-actions bot added Bot A bot generated issue/pull-request Type: Performance labels Feb 28, 2023
@trexfeathers
Copy link
Contributor

The experimental.ugrid benchmarks have lazy and non-lazy equivalents. So many regressions in the lazy ones, with only one in the non-lazy ones (Mesh.time_eq), suggests this is probably related to dask-core-2023.2.1 (previously 2023.2.0).

It could possibly be libnetcdf-4.9.1 (previously 4.8.1), if there's something specific about Iris' lazy access to NetCDF. Could even be an interaction between dask-core and libnetcdf.

Either way some of these are pretty significant changes, and probably not ones we have control over - we can't pin forever. Perhaps the best we can do is warn users in our current What's New page. What does everyone think?

@pp-mo
Copy link
Member

pp-mo commented Feb 28, 2023

some of these are pretty significant changes
I totally agree. This don't look trivial to me, as they include significant %ge increases on several of the larger loading benchmarks.

We should at least check whether this is dask or netcdf ?

@pp-mo
Copy link
Member

pp-mo commented Feb 28, 2023

So many regressions in the lazy ones

Not sure I agree, we have all the load.LoadAndRealise.time_load(xxx) as well as all the load.LoadAndRealise.time_realise(xxx).
Which I think indicates that it is the basic file access for scanning + identifying structure, and not just the variable data access.

If so, would seem to point at netcdf rather than dask ?

@trexfeathers
Copy link
Contributor

Step 1: I can replicate this locally ✔

@trexfeathers
Copy link
Contributor

Step 2: pinning libnetcdf <=4.8.1 removes the regressions, as @pp-mo suspected.

@trexfeathers
Copy link
Contributor

Step 3 (still to do): compare with Xarray to see if they have also experienced regressions (is the problem with us, or with libnetcdf?)

@trexfeathers
Copy link
Contributor

Poll:

@trexfeathers trexfeathers linked a pull request Apr 13, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bot A bot generated issue/pull-request Type: Performance
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants