Merge branch 'feature/DSC-30265-file-pool' into develop

ecmwf · Jan 10, 2025 · 880b047 · 880b047
2 parents 26475e6 + d27f7c6
commit 880b047
Show file tree

Hide file tree

Showing 7 changed files with 110 additions and 18 deletions.
diff --git a/src/eccodes_prototypes.h b/src/eccodes_prototypes.h
@@ -429,6 +429,8 @@ void grib_file_pool_clean(void);
 grib_file* grib_file_pool_get_files(void);
 grib_file* grib_file_open(const char* filename, const char* mode, int* err);
 void grib_file_pool_delete_file(grib_file* file);
+grib_file* grib_file_pool_create_clone(grib_context* c, short clone_id, grib_file* file);
+void grib_file_pool_delete_clone(grib_file* clone);
 void grib_file_close(const char* filename, int force, int* err);
 void grib_file_close_all(int* err);
 grib_file* grib_get_file(const char* filename, int* err);

diff --git a/src/grib_api_internal.h b/src/grib_api_internal.h
@@ -861,6 +861,8 @@ struct grib_file
     char* buffer;
     long refcount;
     grib_file* next;
+    grib_file* pool_file;
+    long pool_file_refcount;
     short id;
 };
 

diff --git a/src/grib_filepool.cc b/src/grib_filepool.cc
@@ -256,6 +256,51 @@ grib_file* grib_file_open(const char* filename, const char* mode, int* err)
     return file;
 }
 
+grib_file* grib_file_pool_create_clone(grib_context* c, short clone_id, grib_file* pool_file)
+{
+    if(pool_file)
+    {
+        grib_file* newfile          = (grib_file*)grib_context_malloc_clear(c, sizeof(grib_file));
+        newfile->id                 = clone_id;
+        newfile->name               = strdup(pool_file->name);
+        newfile->handle             = pool_file->handle;
+        newfile->pool_file          = pool_file;
+        newfile->pool_file_refcount = 0;
+
+        GRIB_MUTEX_INIT_ONCE(&once, &init_mutex);
+        GRIB_MUTEX_LOCK(&mutex1);
+
+        ++pool_file->pool_file_refcount;
+
+        GRIB_MUTEX_UNLOCK(&mutex1);
+
+        return newfile;
+    }
+    else
+        return 0;
+}
+
+void grib_file_pool_delete_clone(grib_file* cloned_file)
+{
+    grib_file* pool_file = cloned_file->pool_file;
+    if(pool_file)
+    {
+        GRIB_MUTEX_INIT_ONCE(&once, &init_mutex);
+        GRIB_MUTEX_LOCK(&mutex1);
+        if(pool_file->pool_file_refcount > 0)
+        {
+            --pool_file->pool_file_refcount;
+
+            if (pool_file->pool_file_refcount == 0)
+                grib_file_pool_delete_file(pool_file);
+        }
+
+        GRIB_MUTEX_UNLOCK(&mutex1);
+    }
+
+    grib_file_delete(cloned_file);
+}
+
 void grib_file_pool_delete_file(grib_file* file)
 {
     grib_file* prev = NULL;
@@ -265,6 +310,7 @@ void grib_file_pool_delete_file(grib_file* file)
     if (file == file_pool.first) {
         file_pool.first   = file->next;
         file_pool.current = file->next;
+        file_pool.size--;
     }
     else {
         prev              = file_pool.first;
@@ -277,10 +323,13 @@ void grib_file_pool_delete_file(grib_file* file)
         DEBUG_ASSERT(prev);
         if (prev) {
             prev->next = file->next;
+            file_pool.size--;
         }
     }
 
     if (file->handle) {
+        fclose(file->handle);
+        file->handle = NULL;
         file_pool.number_of_opened_files--;
     }
     grib_file_delete(file);
@@ -401,12 +450,14 @@ static grib_file* grib_file_new(grib_context* c, const char* name, int* err)
     next_id++;
     GRIB_MUTEX_UNLOCK(&mutex1);
 
-    file->mode     = 0;
-    file->handle   = 0;
-    file->refcount = 0;
-    file->context  = c;
-    file->next     = 0;
-    file->buffer   = 0;
+    file->mode               = 0;
+    file->handle             = 0;
+    file->refcount           = 0;
+    file->context            = c;
+    file->next               = 0;
+    file->pool_file          = 0;
+    file->pool_file_refcount = 0;
+    file->buffer             = 0;
     return file;
 }
 

diff --git a/src/grib_index.cc b/src/grib_index.cc
@@ -767,7 +767,7 @@ void grib_index_delete(grib_index* index)
     while (file) {
         grib_file* f = file;
         file         = file->next;
-        grib_file_delete(f);
+        grib_file_pool_delete_clone(f);
     }
     grib_context_free(index->context, index);
 }
@@ -1076,7 +1076,6 @@ static int codes_index_add_file_internal(grib_index* index, const char* filename
     char buf[1024] = {0,};
     int err = 0;
     grib_file* indfile;
-    grib_file* newfile;
 
     grib_index_key* index_key = NULL;
     grib_handle* h            = NULL;
@@ -1097,11 +1096,7 @@ static int codes_index_add_file_internal(grib_index* index, const char* filename
 
     if (!index->files) {
         grib_filesid++;
-        newfile         = (grib_file*)grib_context_malloc_clear(c, sizeof(grib_file));
-        newfile->id     = grib_filesid;
-        newfile->name   = strdup(file->name);
-        newfile->handle = file->handle;
-        index->files    = newfile;
+        index->files = grib_file_pool_create_clone(c, grib_filesid, file);
     }
     else {
         indfile = index->files;
@@ -1114,11 +1109,7 @@ static int codes_index_add_file_internal(grib_index* index, const char* filename
         while (indfile->next)
             indfile = indfile->next;
         grib_filesid++;
-        newfile         = (grib_file*)grib_context_malloc_clear(c, sizeof(grib_file));
-        newfile->id     = grib_filesid;
-        newfile->name   = strdup(file->name);
-        newfile->handle = file->handle;
-        indfile->next   = newfile;
+        indfile->next = grib_file_pool_create_clone(c, grib_filesid, file);
     }
 
     fseeko(file->handle, 0, SEEK_SET);

diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
@@ -286,6 +286,7 @@ if( HAVE_BUILD_TOOLS )
         grib_ecc-1941
         grib_ecc-1942
         grib_ecc-2002
+        grib_ecc-2006
         grib_modelName
         grib_sub_hourly
         grib_set_bytes

diff --git a/tests/grib_ecc-2006.sh b/tests/grib_ecc-2006.sh
@@ -0,0 +1,39 @@
+#!/bin/sh
+# (C) Copyright 2005- ECMWF.
+#
+# This software is licensed under the terms of the Apache Licence Version 2.0
+# which can be obtained at http://www.apache.org/licenses/LICENSE-2.0.
+# 
+# In applying this licence, ECMWF does not waive the privileges and immunities granted to it by
+# virtue of its status as an intergovernmental organisation nor does it submit to any jurisdiction.
+#
+
+. ./include.ctest.sh
+
+# -------------------------------------------------------------------
+# This is the test for the JIRA issue ECC-2006.
+# Indexing: Performance degradation after reading thousands of files
+# -------------------------------------------------------------------
+
+label="grib_ecc-2006_test"
+
+tempIndex=temp.$label.idx
+tempLog=temp.$label.log
+tempOut=temp.$label.txt
+
+export ECCODES_DEBUG=-1
+
+gfiles="
+    $data_dir/tigge_af_ecmwf.grib2
+    $data_dir/tigge_cf_ecmwf.grib2
+    $data_dir/tigge_ecmwf.grib2
+    $data_dir/tigge_pf_ecmwf.grib2"
+
+${tools_dir}/grib_index_build -o $tempIndex $gfiles >$tempLog 2>&1
+
+# There should be no files in the pool after we call grib_index_delete
+grep "file_pool before grib_index_delete: size=4, num_opened_files=4" $tempLog
+grep "file_pool after grib_index_delete: size=0, num_opened_files=0" $tempLog
+
+# Clean up
+rm -f $tempIndex $tempLog $tempOut
diff --git a/tools/grib_index_build.cc b/tools/grib_index_build.cc
@@ -147,7 +147,13 @@ int grib_tool_finalise_action(grib_runtime_options* options)
 
     if (idx->count)
         grib_index_write(idx, options->outfile->name);
+
+    // See ECC-2006
+    grib_context* c = grib_context_get_default();
+    if (c->debug) grib_file_pool_print("ECCODES DEBUG file_pool before grib_index_delete", stderr);
     grib_index_delete(idx);
+    if (c->debug) grib_file_pool_print("ECCODES DEBUG file_pool after grib_index_delete", stderr);
+
     return 0;
 }