-
-
Notifications
You must be signed in to change notification settings - Fork 30
ENH: unlock the gil during GDAL functions that can take significant time #572
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
Open
theroggy
wants to merge
79
commits into
geopandas:main
Choose a base branch
from
theroggy:ENH-unlock-the-gil-during-execute-of-SQL
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+117
−59
Open
Changes from all commits
Commits
Show all changes
79 commits
Select commit
Hold shift + click to select a range
557763e
Fix: pass kwargs on to OGR write
theroggy 90cf78a
Merge remote-tracking branch 'upstream/main'
theroggy 14c01c5
Merge remote-tracking branch 'upstream/main'
theroggy a924eb0
Merge remote-tracking branch 'upstream/main'
theroggy da65c46
Merge remote-tracking branch 'upstream/main'
theroggy cd1cfd8
Merge remote-tracking branch 'upstream/main'
theroggy 22434dd
Merge remote-tracking branch 'upstream/main'
theroggy 050df67
Merge remote-tracking branch 'upstream/main'
theroggy d999ec3
Merge remote-tracking branch 'upstream/main'
theroggy b92b3c3
Merge remote-tracking branch 'upstream/main'
theroggy 238f151
Merge remote-tracking branch 'upstream/main'
theroggy 1127ad7
Merge remote-tracking branch 'upstream/main'
theroggy 664d8fe
Merge remote-tracking branch 'upstream/main'
theroggy ca87d9b
Merge remote-tracking branch 'upstream/main'
theroggy 813c3f6
Merge remote-tracking branch 'upstream/main'
theroggy f53a0ad
Merge remote-tracking branch 'upstream/main'
theroggy 987f47e
Merge remote-tracking branch 'upstream/main'
theroggy 1ee3968
Merge remote-tracking branch 'upstream/main'
theroggy ed6432a
Merge remote-tracking branch 'upstream/main'
theroggy d89ed55
Merge remote-tracking branch 'upstream/main'
theroggy b050651
Merge remote-tracking branch 'upstream/main'
theroggy 6d52e83
Merge remote-tracking branch 'upstream/main'
theroggy ff9dec9
Merge remote-tracking branch 'upstream/main'
theroggy 32b7580
Merge remote-tracking branch 'upstream/main'
theroggy cf99380
Merge remote-tracking branch 'upstream/main'
theroggy 482373f
Merge remote-tracking branch 'upstream/main'
theroggy fe56ddb
Merge remote-tracking branch 'upstream/main'
theroggy fb856c0
Merge remote-tracking branch 'upstream/main'
theroggy cd7ea7f
Merge remote-tracking branch 'upstream/main'
theroggy 6c0a4c9
Merge branch 'main' of https://github.com/theroggy/pyogrio
theroggy 263d87a
Merge remote-tracking branch 'upstream/main'
theroggy 46602dc
Merge remote-tracking branch 'upstream/main'
theroggy 8e30556
Merge remote-tracking branch 'upstream/main'
theroggy 6b9b690
Merge remote-tracking branch 'upstream/main'
theroggy 3b069df
Merge remote-tracking branch 'upstream/main'
theroggy 2951f0e
Merge remote-tracking branch 'upstream/main'
theroggy 7386097
Merge remote-tracking branch 'upstream/main'
theroggy 2c29995
Merge remote-tracking branch 'upstream/main'
theroggy b7575ac
Merge remote-tracking branch 'upstream/main'
theroggy d7967cc
Merge remote-tracking branch 'upstream/main'
theroggy 0e5d5bc
Merge remote-tracking branch 'upstream/main'
theroggy 89647b4
Merge remote-tracking branch 'upstream/main'
theroggy 84f6874
Merge remote-tracking branch 'upstream/main'
theroggy ff8b1f4
Merge remote-tracking branch 'upstream/main'
theroggy dc26a4e
Merge remote-tracking branch 'upstream/main'
theroggy 8652bd3
Merge remote-tracking branch 'upstream/main'
theroggy 53a8295
Merge remote-tracking branch 'upstream/main'
theroggy 696cf6c
Merge remote-tracking branch 'upstream/main'
theroggy 807a7a8
Merge remote-tracking branch 'upstream/main'
theroggy c199715
Revert "MAINT: avoid tests to run if linting fails (#459)"
theroggy c6af1c8
Merge remote-tracking branch 'upstream/main'
theroggy 94a6b9e
Merge remote-tracking branch 'upstream/main'
theroggy f0fcb25
Merge remote-tracking branch 'upstream/main'
theroggy 9485eca
Merge remote-tracking branch 'upstream/main'
theroggy 9c66fc6
Merge remote-tracking branch 'upstream/main'
theroggy b2055bb
Merge remote-tracking branch 'upstream/main'
theroggy 4f91c36
Merge remote-tracking branch 'upstream/main'
theroggy 9dd7c64
Merge remote-tracking branch 'upstream/main'
theroggy 4b8edd3
Merge remote-tracking branch 'upstream/main'
theroggy c2c38f3
Merge remote-tracking branch 'upstream/main'
theroggy c2d2306
Merge remote-tracking branch 'upstream/main'
theroggy 647d0e8
Merge remote-tracking branch 'upstream/main'
theroggy b2f6b1a
Merge remote-tracking branch 'upstream/main'
theroggy 2ac0044
Merge remote-tracking branch 'upstream/main'
theroggy e614d34
Merge remote-tracking branch 'upstream/main'
theroggy b425e83
Merge remote-tracking branch 'upstream/main'
theroggy 951529a
Merge remote-tracking branch 'upstream/main'
theroggy a153ec3
Merge remote-tracking branch 'upstream/main'
theroggy 80f5a4c
Merge remote-tracking branch 'upstream/main'
theroggy 6f24d6e
Merge remote-tracking branch 'upstream/main'
theroggy b8514e4
Merge remote-tracking branch 'upstream/main'
theroggy b34b903
Merge remote-tracking branch 'upstream/main'
theroggy 6dbef2c
ENH: unlock the gil during execute of SQL
theroggy d833903
Use check_pointer outside nogil
theroggy 7841b82
Update CHANGES.md
theroggy 86b5b86
Use nogil on all GDAL functions that can take significant time
theroggy 9b76da1
Update CHANGES.md
theroggy 7eb3627
Merge remote-tracking branch 'upstream/main' into ENH-unlock-the-gil-…
theroggy 1bc7a4b
Merge remote-tracking branch 'upstream/main' into ENH-unlock-the-gil-…
theroggy File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,7 +15,6 @@ from pathlib import Path | |
|
|
||
| from libc.stdint cimport uint8_t, uintptr_t | ||
| from libc.stdlib cimport malloc, free | ||
| from libc.string cimport strlen | ||
| from libc.math cimport isnan | ||
| from cpython.pycapsule cimport PyCapsule_GetPointer | ||
|
|
||
|
|
@@ -226,6 +225,7 @@ cdef void* ogr_open(const char* path_c, int mode, char** options) except NULL: | |
| """ | ||
| cdef void *ogr_dataset = NULL | ||
| cdef ErrorHandler errors | ||
| cdef int flags | ||
|
|
||
| # Force linear approximations in all cases | ||
| OGRSetNonLinearGeometriesEnabledFlag(0) | ||
|
|
@@ -240,9 +240,10 @@ cdef void* ogr_open(const char* path_c, int mode, char** options) except NULL: | |
| # WARNING: GDAL logs warnings about invalid open options to stderr | ||
| # instead of raising an error | ||
| with capture_errors() as errors: | ||
| ogr_dataset = GDALOpenEx( | ||
| path_c, flags, NULL, <const char *const *>options, NULL | ||
| ) | ||
| with nogil: | ||
| ogr_dataset = GDALOpenEx( | ||
| path_c, flags, NULL, <const char *const *>options, NULL | ||
| ) | ||
| return errors.check_pointer(ogr_dataset, True) | ||
|
|
||
| except NullPointerError: | ||
|
|
@@ -295,15 +296,23 @@ cdef OGRLayerH get_ogr_layer(GDALDatasetH ogr_dataset, layer) except NULL: | |
| pointer to OGR layer | ||
| """ | ||
| cdef OGRLayerH ogr_layer = NULL | ||
| cdef char *layer_c | ||
| cdef int layer_int | ||
| cdef char *sql_c | ||
|
|
||
| try: | ||
| if isinstance(layer, str): | ||
| name_b = layer.encode("utf-8") | ||
| name_c = name_b | ||
| ogr_layer = check_pointer(GDALDatasetGetLayerByName(ogr_dataset, name_c)) | ||
| layer_b = layer.encode("utf-8") | ||
| layer_c = layer_b | ||
| with nogil: | ||
| ogr_layer = GDALDatasetGetLayerByName(ogr_dataset, layer_c) | ||
| ogr_layer = check_pointer(ogr_layer) | ||
|
|
||
| elif isinstance(layer, int): | ||
| ogr_layer = check_pointer(GDALDatasetGetLayer(ogr_dataset, layer)) | ||
| layer_int = layer | ||
| with nogil: | ||
| ogr_layer = GDALDatasetGetLayer(ogr_dataset, layer_int) | ||
| ogr_layer = check_pointer(ogr_layer) | ||
| else: | ||
| raise ValueError( | ||
| f"'layer' parameter must be a str or int, got {type(layer)}" | ||
|
|
@@ -325,7 +334,8 @@ cdef OGRLayerH get_ogr_layer(GDALDatasetH ogr_dataset, layer) except NULL: | |
| sql_b = f"SET interest_layers = {layer_name}".encode("utf-8") | ||
| sql_c = sql_b | ||
|
|
||
| GDALDatasetExecuteSQL(ogr_dataset, sql_c, NULL, NULL) | ||
| with nogil: | ||
| GDALDatasetExecuteSQL(ogr_dataset, sql_c, NULL, NULL) | ||
|
|
||
| return ogr_layer | ||
|
|
||
|
|
@@ -347,18 +357,23 @@ cdef OGRLayerH execute_sql( | |
| ------- | ||
| pointer to OGR layer | ||
| """ | ||
| cdef char * sql_c | ||
| cdef char * sql_dialect_c | ||
| cdef OGRLayerH retval = NULL | ||
|
|
||
| try: | ||
| sql_b = sql.encode("utf-8") | ||
| sql_c = sql_b | ||
| if sql_dialect is None: | ||
| return check_pointer(GDALDatasetExecuteSQL(ogr_dataset, sql_c, NULL, NULL)) | ||
| with nogil: | ||
| retval = GDALDatasetExecuteSQL(ogr_dataset, sql_c, NULL, NULL) | ||
| return check_pointer(retval) | ||
|
|
||
| sql_dialect_b = sql_dialect.encode("utf-8") | ||
| sql_dialect_c = sql_dialect_b | ||
| return check_pointer(GDALDatasetExecuteSQL( | ||
| ogr_dataset, sql_c, NULL, sql_dialect_c) | ||
| ) | ||
| with nogil: | ||
| retval = GDALDatasetExecuteSQL(ogr_dataset, sql_c, NULL, sql_dialect_c) | ||
| return check_pointer(retval) | ||
|
|
||
| # GDAL does not always raise exception messages in this case | ||
| except NullPointerError: | ||
|
|
@@ -386,7 +401,9 @@ cdef str get_crs(OGRLayerH ogr_layer): | |
| cdef char *ogr_wkt = NULL | ||
|
|
||
| try: | ||
| ogr_crs = check_pointer(OGR_L_GetSpatialRef(ogr_layer)) | ||
| with nogil: | ||
| ogr_crs = OGR_L_GetSpatialRef(ogr_layer) | ||
| ogr_crs = check_pointer(ogr_crs) | ||
|
|
||
| except NullPointerError: | ||
| # No coordinate system defined. | ||
|
|
@@ -433,7 +450,9 @@ cdef get_driver(OGRDataSourceH ogr_dataset): | |
| cdef void *ogr_driver | ||
|
|
||
| try: | ||
| ogr_driver = check_pointer(GDALGetDatasetDriver(ogr_dataset)) | ||
| with nogil: | ||
| ogr_driver = GDALGetDatasetDriver(ogr_dataset) | ||
| ogr_driver = check_pointer(ogr_driver) | ||
|
|
||
| except NullPointerError: | ||
| raise DataLayerError(f"Could not detect driver of dataset") from None | ||
|
|
@@ -464,19 +483,25 @@ cdef get_feature_count(OGRLayerH ogr_layer, int force): | |
| """ | ||
|
|
||
| cdef OGRFeatureH ogr_feature = NULL | ||
| cdef int feature_count = OGR_L_GetFeatureCount(ogr_layer, force) | ||
| cdef int feature_count | ||
|
|
||
| with nogil: | ||
| feature_count = OGR_L_GetFeatureCount(ogr_layer, force) | ||
|
|
||
| # if GDAL refuses to give us the feature count, we have to loop over all | ||
| # features ourselves and get the count. This can happen for some drivers | ||
| # (e.g., OSM) or if a where clause is invalid but not rejected as error | ||
| if force and feature_count == -1: | ||
| # make sure layer is read from beginning | ||
| OGR_L_ResetReading(ogr_layer) | ||
| with nogil: | ||
| OGR_L_ResetReading(ogr_layer) | ||
|
|
||
| feature_count = 0 | ||
| while True: | ||
| try: | ||
| ogr_feature = check_pointer(OGR_L_GetNextFeature(ogr_layer)) | ||
| with nogil: | ||
| ogr_feature = OGR_L_GetNextFeature(ogr_layer) | ||
| ogr_feature = check_pointer(ogr_feature) | ||
| feature_count +=1 | ||
|
|
||
| except NullPointerError: | ||
|
|
@@ -519,10 +544,11 @@ cdef get_total_bounds(OGRLayerH ogr_layer, int force): | |
| tuple of (xmin, ymin, xmax, ymax) or None | ||
| The total bounds of the layer, or None if they could not be determined. | ||
| """ | ||
|
|
||
| cdef OGREnvelope ogr_envelope | ||
|
|
||
| if OGR_L_GetExtent(ogr_layer, &ogr_envelope, force) == OGRERR_NONE: | ||
| with nogil: | ||
| err = OGR_L_GetExtent(ogr_layer, &ogr_envelope, force) | ||
| if err == OGRERR_NONE: | ||
| bounds = ( | ||
| ogr_envelope.MinX, ogr_envelope.MinY, ogr_envelope.MaxX, ogr_envelope.MaxY | ||
| ) | ||
|
|
@@ -677,7 +703,9 @@ cdef get_fields(OGRLayerH ogr_layer, str encoding, use_arrow=False): | |
| cdef const char *key_c | ||
|
|
||
| try: | ||
| ogr_featuredef = check_pointer(OGR_L_GetLayerDefn(ogr_layer)) | ||
| with nogil: | ||
| ogr_featuredef = OGR_L_GetLayerDefn(ogr_layer) | ||
| ogr_featuredef = check_pointer(ogr_featuredef) | ||
|
|
||
| except NullPointerError: | ||
| raise DataLayerError("Could not get layer definition") from None | ||
|
|
@@ -751,10 +779,14 @@ cdef apply_where_filter(OGRLayerH ogr_layer, str where): | |
| ------ | ||
| ValueError: if SQL query is not valid | ||
| """ | ||
| cdef const char* where_c | ||
| cdef int err | ||
|
|
||
| where_b = where.encode("utf-8") | ||
| where_c = where_b | ||
| err = OGR_L_SetAttributeFilter(ogr_layer, where_c) | ||
| with nogil: | ||
| err = OGR_L_SetAttributeFilter(ogr_layer, where_c) | ||
|
|
||
| # WARNING: GDAL does not raise this error for GPKG but instead only | ||
| # logs to stderr | ||
| if err != OGRERR_NONE: | ||
|
|
@@ -781,12 +813,14 @@ cdef apply_bbox_filter(OGRLayerH ogr_layer, bbox): | |
| ValueError: if bbox is not a list or tuple or does not have proper number of | ||
| items | ||
| """ | ||
| cdef double xmin, ymin, xmax, ymax | ||
|
|
||
| if not (isinstance(bbox, (tuple, list)) and len(bbox) == 4): | ||
| raise ValueError(f"Invalid bbox: {bbox}") | ||
|
|
||
| xmin, ymin, xmax, ymax = bbox | ||
| OGR_L_SetSpatialFilterRect(ogr_layer, xmin, ymin, xmax, ymax) | ||
| with nogil: | ||
| OGR_L_SetSpatialFilterRect(ogr_layer, xmin, ymin, xmax, ymax) | ||
|
|
||
|
|
||
| cdef apply_geometry_filter(OGRLayerH ogr_layer, wkb): | ||
|
|
@@ -807,7 +841,8 @@ cdef apply_geometry_filter(OGRLayerH ogr_layer, wkb): | |
| OGR_G_DestroyGeometry(ogr_geometry) | ||
| raise GeometryError("Could not create mask geometry") from None | ||
|
|
||
| OGR_L_SetSpatialFilter(ogr_layer, ogr_geometry) | ||
| with nogil: | ||
| OGR_L_SetSpatialFilter(ogr_layer, ogr_geometry) | ||
| OGR_G_DestroyGeometry(ogr_geometry) | ||
|
|
||
|
|
||
|
|
@@ -819,7 +854,9 @@ cdef apply_skip_features(OGRLayerH ogr_layer, int skip_features): | |
| ogr_layer : pointer to open OGR layer | ||
| wskip_features : int | ||
| """ | ||
| err = OGR_L_SetNextByIndex(ogr_layer, skip_features) | ||
| with nogil: | ||
| err = OGR_L_SetNextByIndex(ogr_layer, skip_features) | ||
|
|
||
| # GDAL can raise an error (depending on the format) for out-of-bound index, | ||
| # but `validate_feature_range()` should ensure we only pass a valid number | ||
| if err != OGRERR_NONE: | ||
|
|
@@ -1076,14 +1113,14 @@ cdef get_features( | |
| uint8_t return_fids, | ||
| bint datetime_as_string | ||
| ): | ||
|
|
||
| cdef OGRFeatureH ogr_feature = NULL | ||
| cdef int n_fields | ||
| cdef int i | ||
| cdef int field_index | ||
|
|
||
| # make sure layer is read from beginning | ||
| OGR_L_ResetReading(ogr_layer) | ||
| with nogil: | ||
| OGR_L_ResetReading(ogr_layer) | ||
|
|
||
| if skip_features > 0: | ||
| apply_skip_features(ogr_layer, skip_features) | ||
|
|
@@ -1128,7 +1165,9 @@ cdef get_features( | |
| break | ||
|
|
||
| try: | ||
| ogr_feature = check_pointer(OGR_L_GetNextFeature(ogr_layer)) | ||
| with nogil: | ||
| ogr_feature = OGR_L_GetNextFeature(ogr_layer) | ||
|
Comment on lines
+1168
to
+1169
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar comment here, although in this case we can't really move it up (and a few similar cases below as well) |
||
| ogr_feature = check_pointer(ogr_feature) | ||
|
|
||
| except NullPointerError: | ||
| # No more rows available, so stop reading | ||
|
|
@@ -1196,7 +1235,8 @@ cdef get_features_by_fid( | |
| cdef int count = len(fids) | ||
|
|
||
| # make sure layer is read from beginning | ||
| OGR_L_ResetReading(ogr_layer) | ||
| with nogil: | ||
| OGR_L_ResetReading(ogr_layer) | ||
|
|
||
| if read_geometry: | ||
| geometries = np.empty(shape=(count, ), dtype="object") | ||
|
|
@@ -1226,7 +1266,9 @@ cdef get_features_by_fid( | |
| fid = fids[i] | ||
|
|
||
| try: | ||
| ogr_feature = check_pointer(OGR_L_GetFeature(ogr_layer, fid)) | ||
| with nogil: | ||
| ogr_feature = OGR_L_GetFeature(ogr_layer, fid) | ||
| ogr_feature = check_pointer(ogr_feature) | ||
|
|
||
| except NullPointerError: | ||
| raise FeatureError(f"Could not read feature with fid {fid}") from None | ||
|
|
@@ -1258,7 +1300,8 @@ cdef get_bounds(OGRLayerH ogr_layer, int skip_features, int num_features): | |
| cdef int i | ||
|
|
||
| # make sure layer is read from beginning | ||
| OGR_L_ResetReading(ogr_layer) | ||
| with nogil: | ||
| OGR_L_ResetReading(ogr_layer) | ||
|
|
||
| if skip_features > 0: | ||
| apply_skip_features(ogr_layer, skip_features) | ||
|
|
@@ -1276,7 +1319,9 @@ cdef get_bounds(OGRLayerH ogr_layer, int skip_features, int num_features): | |
| break | ||
|
|
||
| try: | ||
| ogr_feature = check_pointer(OGR_L_GetNextFeature(ogr_layer)) | ||
| with nogil: | ||
| ogr_feature = OGR_L_GetNextFeature(ogr_layer) | ||
| ogr_feature = check_pointer(ogr_feature) | ||
|
|
||
| except NullPointerError: | ||
| # No more rows available, so stop reading | ||
|
|
@@ -1457,7 +1502,8 @@ def ogr_read( | |
| field_c = field_b | ||
| fields_c = CSLAddString(fields_c, field_c) | ||
|
|
||
| OGR_L_SetIgnoredFields(ogr_layer, <const char**>fields_c) | ||
| with nogil: | ||
| OGR_L_SetIgnoredFields(ogr_layer, <const char**>fields_c) | ||
|
|
||
| geometry_type = get_geometry_type(ogr_layer) | ||
|
|
||
|
|
@@ -1799,7 +1845,8 @@ def ogr_open_arrow( | |
| field_c = field_b | ||
| fields_c = CSLAddString(fields_c, field_c) | ||
|
|
||
| OGR_L_SetIgnoredFields(ogr_layer, <const char**>fields_c) | ||
| with nogil: | ||
| OGR_L_SetIgnoredFields(ogr_layer, <const char**>fields_c) | ||
|
|
||
| if not return_fids: | ||
| options = CSLSetNameValue(options, "INCLUDE_FID", "NO") | ||
|
|
@@ -1820,20 +1867,24 @@ def ogr_open_arrow( | |
| ) | ||
|
|
||
| # make sure layer is read from beginning | ||
| OGR_L_ResetReading(ogr_layer) | ||
| with nogil: | ||
| OGR_L_ResetReading(ogr_layer) | ||
|
|
||
| # allocate the stream struct and wrap in capsule to ensure clean-up on error | ||
| capsule = alloc_c_stream(&stream) | ||
|
|
||
| if not OGR_L_GetArrowStream(ogr_layer, stream, options): | ||
| with nogil: | ||
| reval = OGR_L_GetArrowStream(ogr_layer, stream, options) | ||
| if not reval: | ||
| raise RuntimeError("Failed to open ArrowArrayStream from Layer") | ||
|
|
||
| if skip_features > 0: | ||
| # only supported for GDAL >= 3.8.0; have to do this after getting | ||
| # the Arrow stream | ||
| # use `OGR_L_SetNextByIndex` directly and not `apply_skip_features` | ||
| # to ignore errors in case skip_features == feature_count | ||
| OGR_L_SetNextByIndex(ogr_layer, skip_features) | ||
| with nogil: | ||
| OGR_L_SetNextByIndex(ogr_layer, skip_features) | ||
|
|
||
| if use_pyarrow: | ||
| import pyarrow as pa | ||
|
|
@@ -2142,12 +2193,14 @@ cdef get_layer_names(OGRDataSourceH ogr_dataset): | |
| """ | ||
| cdef OGRLayerH ogr_layer = NULL | ||
|
|
||
| layer_count = GDALDatasetGetLayerCount(ogr_dataset) | ||
| with nogil: | ||
| layer_count = GDALDatasetGetLayerCount(ogr_dataset) | ||
|
|
||
| data = np.empty(shape=(layer_count, 2), dtype=object) | ||
| data_view = data[:] | ||
| for i in range(layer_count): | ||
| ogr_layer = GDALDatasetGetLayer(ogr_dataset, i) | ||
| with nogil: | ||
| ogr_layer = GDALDatasetGetLayer(ogr_dataset, i) | ||
|
|
||
| data_view[i, 0] = get_string(OGR_L_GetName(ogr_layer)) | ||
| data_view[i, 1] = get_geometry_type(ogr_layer) | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are in a loop here for counting the features, this means for a file with 10,000 rows, we are going to release/acquire the GIL 10,000 times?
(if I am not misreading the code, that might not be optimal)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could probably move the
with nogilup, though, and put the entire loop in here. We just need to acquire the GIL againwith gil:when raising the errorsUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jorisvandenbossche
I reran my performance tests with and without having
with nogilaround functions likeOGR_L_GetNextFeaturein the feature-getting, very high quantity loops using the nz building outlines file.Conclusion: it doesn't seem to give a significant difference either way:
It seems the cost of having it there is not significant, at least in the tests I did. There might be file formats where having the nogil there could be beneficial, but this is pure speculation. Also possible that the nogil behaves a bit different on a loaded server than on a windows desktop, also specfulation.
Hence, I don't have any preference for keeping it or not.
Remark: moving it up, so the entire loop is nogilled takes some effort as check_pointer returns a python object (an exception), which isn't trivial to change... and as there doesn't seem to be a performance impact anyway I don't think it is worth the trouble.
With
nogil(also) in the feature reading loops (e.g. aroundOGR_L_GetNextFeature)Without
nogilin the feature-reading loopsScript used: