Image initialization speedup #684

yixinmao · 2017-02-17T20:53:06Z

This PR speeds up image driver initialization by only opening and closing each input netCDF file once (instead of opening and closing when reading every variable). This PR is related to issue #538 , issue #380 and PR #564 .

Specifically:

Originally in the filenames structure, all input file information is saved as filenames only. Now in this PR, for those input files in netCDF format (domain, parameter, initial state and forcings), both filename and nc_id are saved in a new nameid_struct in the filenames structure. This change allows nc_id to be stored alongside with filename once an input nc file is open, and be directly used later.
Removed the opening and closing netCDF file parts from the low-level nc functions, including get_nc_field_<TYPE>, get_nc_var_attr, get_nc_var_type, get_nc_vardimensions. Instead, these functions assume that the nc file is already open beforehand, and take nameid_struct (which includes both filename and nc_id) directly as input argument. Created separate functions for opening and closing nc files.
For domain, parameter and initial state input files, the files are opened before their first use; and are kept open; and are closed after their final use. For forcing files, the first-year file is opened before being used in get_global_param, and kept open; then in vic_force, close the previous-year forcing file and open a new-year file if going into a new year; the last-year file is closed after the last simulation timestep.

Further things to do:

Format clean up
Right now no further double checks are being done to ensure files are indeed opened/closed correctly (the image driver runs successfully though). Do we want to, for example, add check in the end of VIC simulation (or somewhere else) whether all input netCDF files are correctly closed? Or, when reading each variable, double check whether the netCDF file is indeed open? ...
Other updates after code review

Note:

This PR does not include changes for CESM driver. Additional changes are needed for CESM driver to run with the speedup.
This PR only speeds up opening and closing input netCDF files, but not output files.

were missing in PR 671

…forceskip_reset_missed

opening and closing files again and again - Runs successfully without MPI specified - Error with MPI

works

commands

…it_speedup Conflicts: vic/drivers/image/src/vic_force.c

yixinmao · 2017-02-17T21:08:04Z

Some speed tests (times shown below are all walltime):

Test 1 (a very small domain): Stehekin (16 grid cells), 10 day, hourly timestep; hydra master node, 1 processor:

	Before	After
Init time (sec)	23.01	0.08
Run time (sec)	10.98	1.74
Final time (sec)	0.0014	0.0018
Total time (sec)	33.99	1.83
Model cost (pe-hrs/simulated_year)	0.34	0.019

Test 2 (a regional-scale domain): Arkansas Red (3999 grid cells), 10 years, 3 hourly timestep; hyak, 1 node (16 processors total):

	Before	After
Init time (sec)	20.78	1.27
Run time (sec)	5250.93	1083.79
Final time (sec)	0.097	0.045
Total time (sec)	5271.8	1085.1
Model cost (pe-hrs/simulated_year)	2.34	0.48

Test 2 (continental-scale domain): whole CONUS (333579 grid cells), 1 year, 3 hourly timestep; hyak, 2 nodes (32 processors total):

	Before	After
Init time (sec)	76.49	15.48
Run time (sec)	7009.55	6914.95
Final time (sec)	0.20	0.21
Total time (sec)	7166.23	6930.64
Model cost (pe-hrs/simulated_year)	63.70	61.61

It seems like the initialization time is improved quite a lot; the run time is also improved since now we don't open and close the forcing file for each variable at each timestep. The speedup is more significant for smaller basins.

yixinmao · 2017-02-17T21:13:20Z

@jhamman @bartnijssen @dgergel This PR can use a code review now. The CESM test is failing right now since corresponding changes in CESM driver are needed. Other tests have passed, so I think I can incorporate your comments/suggestions first, and then @dgergel can work on the CESM driver update.

bartnijssen · 2017-02-21T22:47:16Z

vic/drivers/shared_image/include/vic_driver_shared_image.h

    char result_dir[MAXSTRING];    /**< directory where results will be written */
    char statefile[MAXSTRING];     /**< name of file in which to store model state */
    char log_path[MAXSTRING];      /**< Location to write log file to */
 } filenames_struct;

-void add_nveg_to_global_domain(char *nc_name, domain_struct *global_domain);
+void add_nveg_to_global_domain(nameid_struct nc_nameid,


Should pass pointers to structures rather than structures. Passing structures means a copy which is more expensive. If you are concerned that the values are changed you can use consts.

So

void add_nveg_to_global_domain(nameid_struct *nc_nameid,

Thanks for pointing this out. I did see from somewhere else that we should always pass pointer of a structure, but didn't do it in the initial attempt. Will make the change.

bartnijssen · 2017-02-21T22:48:22Z

vic/drivers/shared_image/include/vic_driver_shared_image.h

 void free_force(force_data_struct *force);
 void free_veg_hist(veg_hist_struct *veg_hist);
 void get_domain_type(char *cmdstr);
-size_t get_global_domain(char *domain_nc_name, char *param_nc_name,
+size_t get_global_domain(nameid_struct domain_nc_nameid,
+                         nameid_struct param_nc_nameid,
                         domain_struct *global_domain);


All of these should be nameid_struct *. Within the function you would then use a -> rather than a .

bartnijssen · 2017-02-21T22:50:12Z

vic/drivers/shared_image/include/vic_mpi.h

+ *  * @brief   This structure stores netCDF file name and corresponding nc_id
+ *   *****************************************************************************/
+typedef struct {
+    char    nc_file[MAXSTRING];


Since it is a nameid struct, I would call use nc_filename rather than nc_file. The latter is ambiguous, since I would have expect nc_file to be of type FILE * rather than char *

bartnijssen · 2017-02-21T22:53:34Z

vic/drivers/shared_image/src/nc_utils.c

+ * @brief    Open a netCDF file
+ *****************************************************************************/
+int
+open_nc(char *nc_name)


Change function names - this is too close to nc_open and nc_close. Also I don't quite understand why we need the wrapper functions, since not much happens here other than a call to nc_open. So

a) motivate why we need these functions
b) if we need them, change their names.

Actually I don't think we really need these functions - will delete.

bartnijssen · 2017-02-21T23:00:26Z

vic/drivers/image/src/vic_image_start.c

+    if (mpi_rank == VIC_MPI_ROOT) {
+        // Close domain file
+        close_nc(filenames.domain);
+    }


Unclear why we are doing this here rather than in vic_start(). Can we not do this inside the VIC_MPI_ROOT block in vic_start: https://github.com/UW-Hydro/VIC/blob/develop/vic/drivers/shared_image/src/vic_start.c#L69

I really want to avoid loading this information here if it is not needed at this level.

Yes we can do this opening and closing domain file thing entirely inside vic_start. Do you think we should do the same thing for opening and closing parameter file as well (although we do need to open the parameter file in vic_start, and close it in vic_init, if that's what we want to do, since the parameter file variables are used in both functions)?

Basically if the information is not needed at a higher level in the code then it is better to hide it (put it "lower" in the code) and put the code where it is used. We want to keep the high level functions as clean as possible. Also keep in mind that opening the parameter file once or twice is not all that expensive. It's expensive when we have to open it for every read.

yixinmao · 2017-02-22T22:20:49Z

This PR should be merged after PR #685

Conflicts: vic/drivers/image/src/get_global_param.c

including: - pass the pointer of the new nameid_struct variables into functions (instead of passing in the structure itself) - rename the filename element in the nameid_struct from "nc_file" to "nc_filename" - remove the "open_nc" and "close_nc" wrapping functions - move the opening and closing of the domain and parameter nc files into either "vic_start" or "vic_init"

yixinmao · 2017-02-22T23:16:29Z

Have addressed @bartnijssen 's comments

bartnijssen · 2017-02-22T23:36:39Z

vic/drivers/image/src/vic_force.c

@@ -89,7 +104,7 @@ vic_force(void)
    for (j = 0; j < NF; j++) {
        d3start[0] = global_param.forceskip[0] + global_param.forceoffset[0] +
                     j;
-        get_scatter_nc_field_double(filenames.forcing[0],
+        get_scatter_nc_field_double(&(filenames.forcing[0]),


Why are you doing &(filenames.forcing[0])? Isn't filenames.forcing[0] already a string (char *)? I think that you can revert all these to filenames.forcing[0].

Oh I see. filenames.forcing[0] is of type nameid_struct

Yes you are right!

bartnijssen · 2017-02-22T23:41:52Z

vic/drivers/shared_image/include/vic_mpi.h

+    char    nc_filename[MAXSTRING];
+    int     nc_id;
+} nameid_struct;
+


I think that this needs to be moved to vic_driver_shared_image.h where all the other netcdf related structures are defined.

The reason I put this here is that vic_mpi.h is lower level than vic_driver_shared_image.h (i.e., vic_driver_shared_image.h includes vic_mpi.h, but not vice versa); but the get_scatter_nc_field_XX functions in vic_mpi.h needs to use the nameid_struct structure. I agree that it's not super clean - do you have any suggestion?

Well - the nameid_struct is not specific to the use of mpi so it does not really belong there. It's specific to the use of netcdf. Let me think about it.

OK - I see what you are saying - I will accept the PR and then quickly create a new one in which all the netcdf function definitions, etc are collected in a separate vic_netcdf.h header

I tried a few things but nothing works out nicely. I think this is a bit of a design flaw here, but I don't see an easy fix. I still don't quite think it belongs where we put it, but we'll leave it be for now.

OK - thanks for looking into this.

yixinmao · 2017-02-27T02:19:55Z

New speed tests with time for vic_force and vic_write_output (times shown below are all walltime):

Test 1 (a very small domain): Stehekin (16 grid cells), 10 day, hourly timestep; hydra master node, 1 processor:

	Before	After
Init time (sec)	23.08	0.087
Run time (sec)	11.02	1.75
Final time (sec)	0.0016	0.012
Total time (sec)	34.10	1.85
Model cost (pe-hrs/simulated_year)	0.346	0.0187
vic_force (sec)	9.32	0.064
vic_write_output (sec)	0.170	0.177

Test 2 (a regional-scale domain): Arkansas Red (3999 grid cells), 10 years, 3 hourly timestep; hyak, 1 node (16 processors total):

	Before	After
Init time (sec)	13.2	0.95
Run time (sec)	1950.8	1107.9
Final time (sec)	0.02	0.043
Total time (sec)	1964.0	1108.9
Model cost (pe-hrs/simulated_year)	0.87	0.49
vic_force (sec)	1420.6	548.3
vic_write_output (sec)	119.8	125.1

Test 2 (continental-scale domain): whole CONUS (333579 grid cells), 1 year, 3 hourly timestep; hyak, 2 nodes (32 processors total):

	Before	After
Init time (sec)	73.6	16.1
Run time (sec)	6989.7	6949.6
Final time (sec)	0.22	0.20
Total time (sec)	7063.5	6966.0
Model cost (pe-hrs/simulated_year)	62.8	61.9
vic_force (sec)	1284.4	802.7
vic_write_output (sec)	13.5	11.2

The times in this set of tests are somewhat different from last test because of variation of different VIC run realizations (even if we use the same specs and machines). But note that the "before" time for the 10-year Arkansas-Red run in our last test showed a much longer running time - I might have made some mistakes last time.

ymao added 11 commits February 16, 2017 11:26

Deleted unused file handles (for text files) for nc input files

d7f0cff

Fixed some remaining bugs related to reseting forceskip to zero that

8a9a4cf

were missing in PR 671

Merge branch 'fix_bug_forceskip_reset_missed' into image_init_speedup

46871aa

Merge remote-tracking branch 'remotes/upstream/develop' into fix_bug_…

f525f09

…forceskip_reset_missed

Merge branch 'fix_bug_forceskip_reset_missed' into image_init_speedup

205777d

Fixed a typo

fbbf076

Merge branch 'fix_bug_forceskip_reset_missed' into image_init_speedup

861671d

First commit for speeding up image driver initialization - by not

0c3609f

opening and closing files again and again - Runs successfully without MPI specified - Error with MPI

Image initialization speedup - fixed MPI related bugs so now MPI run

de656cd

works

Fixed bugs corresponding to initialization speedup in some printing

1f3a85d

commands

Merge remote-tracking branch 'remotes/upstream/develop' into image_in…

8b41d5a

…it_speedup Conflicts: vic/drivers/image/src/vic_force.c

bartnijssen mentioned this pull request Feb 21, 2017

Enhancement/netcdf speed #564

Closed

bartnijssen requested changes Feb 21, 2017

View reviewed changes

ymao added 2 commits February 22, 2017 14:22

Merge branch 'fix_bug_image_forcing2' into image_init_speedup

8e2019e

Conflicts: vic/drivers/image/src/get_global_param.c

Merge branch 'develop' into image_init_speedup

d0fb240

bartnijssen requested changes Feb 22, 2017

View reviewed changes

bartnijssen approved these changes Feb 23, 2017

View reviewed changes

bartnijssen merged commit 2993b4f into UW-Hydro:develop Feb 23, 2017

bartnijssen mentioned this pull request Feb 23, 2017

Implement netcdf speedup in cesm driver #686

Closed

bartnijssen mentioned this pull request Feb 28, 2017

ENH: Keep netCDF files open #380

Closed

dgergel mentioned this pull request Mar 3, 2017

Feature/implement runcell changes and netcdf speedup cesm driver #692

Merged

jhamman mentioned this pull request Mar 3, 2017

Accelerating VIC with Open-MP / Manycore architectures. #693

Closed

yixinmao mentioned this pull request Mar 4, 2017

Routing extension for VIC image driver. #231

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Image initialization speedup #684

Image initialization speedup #684

yixinmao commented Feb 17, 2017

yixinmao commented Feb 17, 2017

yixinmao commented Feb 17, 2017

bartnijssen Feb 21, 2017

yixinmao Feb 22, 2017

bartnijssen Feb 21, 2017

bartnijssen Feb 21, 2017

bartnijssen Feb 21, 2017

yixinmao Feb 22, 2017

bartnijssen Feb 21, 2017

yixinmao Feb 22, 2017

bartnijssen Feb 22, 2017

yixinmao commented Feb 22, 2017

yixinmao commented Feb 22, 2017

bartnijssen Feb 22, 2017

bartnijssen Feb 22, 2017

yixinmao Feb 23, 2017

bartnijssen Feb 22, 2017

yixinmao Feb 23, 2017

bartnijssen Feb 23, 2017

bartnijssen Feb 23, 2017

bartnijssen Feb 23, 2017

yixinmao Feb 23, 2017

yixinmao commented Feb 27, 2017

Image initialization speedup #684

Image initialization speedup #684

Conversation

yixinmao commented Feb 17, 2017

yixinmao commented Feb 17, 2017

yixinmao commented Feb 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yixinmao commented Feb 22, 2017

yixinmao commented Feb 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yixinmao commented Feb 27, 2017