mathieu17g
commented
2 years ago
- Include field and geomfield names in feature definition type
FDTypeto avoid callingogr_f_getfieldindex
I have changed the FeatureDefn type parameter of ArchGDAL OGR objects from
FDType = Tuple{NTuple{NG,GType} where NG,NTuple{NF,FType} where NF}which is the same as
FDType = Tuple{
Tuple{Vararg{GType, NG}} where NG,
Tuple{Vararg{FType, NF}} where NF,
}to (thanks to https://discourse.julialang.org/t/parametric-type-with-a-constrained-namedtuple/34511)
FDType = Tuple{
NamedTuple{NG,<:Tuple{Vararg{GType}}} where NG,
NamedTuple{NF,<:Tuple{Vararg{FType}}} where NF,
}
- a way to dispatch on type in
getfield(to fix issue #246) which brings 5% performance gain to the latter
The total performance gain with with the addition of avoiding most of the calls to ogr_f_getfieldindex is now 10% compared to 1.
Current master:
@benchmark Tables.columntable(AG.getlayer(AG.read("../../tmp/road/road.shp"), 0)) seconds = 30
BenchmarkTools.Trial: 142 samples with 1 evaluation.
Range (min … max): 182.404 ms … 335.852 ms ┊ GC (min … max): 0.00% … 10.29%
Time (median): 196.571 ms ┊ GC (median): 0.00%
Time (mean ± σ): 213.022 ms ± 37.221 ms ┊ GC (mean ± σ): 3.27% ± 4.88%
▃█▅▄▂▄ ▄
▆██████▇██▇▃▃▁▃▁▃▁▃▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▃▁▃▁▁▃▃▅▄▇▁▃▄▆▃▅▃▁▁▁▁▁▁▁▁▁▃ ▃
182 ms Histogram: frequency by time 318 ms <
Memory estimate: 8.98 MiB, allocs estimate: 373869.Shapefile.jl:
julia> @benchmark Tables.columntable(Shapefile.Table("../../tmp/road/road.shp")) seconds = 30BenchmarkTools.Trial: 198 samples with 1 evaluation.
Range (min … max): 142.824 ms … 163.894 ms ┊ GC (min … max): 0.00% … 5.48%
Time (median): 152.970 ms ┊ GC (median): 4.10%
Time (mean ± σ): 152.064 ms ± 4.070 ms ┊ GC (mean ± σ): 2.95% ± 2.46%
▃▂▂▄ ▄ ▇▂█▂▄▄▅
▃▁▁▃▁▃▃▅▆▅▆▆▅█▇█████▅██▃▃▅▃▃▅▁█▆██▆▇████████▆▃▆▇▇▆▅▅▁▁▁▃▃▁▁▁▃ ▃
143 ms Histogram: frequency by time 161 ms <
Memory estimate: 35.89 MiB, allocs estimate: 243040.Current master with geometry stealing:
julia> @benchmark Tables.columntable(AG.getlayer(AG.read("../../tmp/road/road.shp"), 0)) seconds = 30
BenchmarkTools.Trial: 144 samples with 1 evaluation.
Range (min … max): 178.243 ms … 312.567 ms ┊ GC (min … max): 0.00% … 13.64%
Time (median): 191.755 ms ┊ GC (median): 0.00%
Time (mean ± σ): 208.443 ms ± 40.801 ms ┊ GC (mean ± σ): 3.39% ± 5.23%
█▂▁▆▅▂▂
▃████████▅▄▃▃▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▃▁▁▃▁▁▃▃▃▄▃▅▁▃▄▃▃▃▃ ▃
178 ms Histogram: frequency by time 312 ms <
Memory estimate: 8.98 MiB, allocs estimate: 373879.New ArchGDAL OGR types with geometry stealing:
julia> @benchmark Tables.columntable(AG.getFDPlayer(AG.read("../../tmp/road/road.shp"), 0)) seconds = 30
BenchmarkTools.Trial: 169 samples with 1 evaluation.
Range (min … max): 158.547 ms … 245.862 ms ┊ GC (min … max): 0.00% … 7.86%
Time (median): 169.471 ms ┊ GC (median): 0.00%
Time (mean ± σ): 178.152 ms ± 22.915 ms ┊ GC (mean ± σ): 1.48% ± 2.67%
▁▃▃▆▄▇█▁▃▂▆▁
▇████████████▇▄▃▁▁▃▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▃▁▁▁▁▁▃▅▇▃▃▆▄▅▃▃▁▃▁▁▁▃ ▃
159 ms Histogram: frequency by time 243 ms <
Memory estimate: 7.75 MiB, allocs estimate: 279394.Beyond the call to ogr_l_getnextfeature, the last step taking some time is the IGeometry constructor
yeesian
visr
quinnj
maxfreu
rafaqz
Here is a draft for review, demonstrating:
ogr_f_stealgeometryfor a gain of an additionnal 7% gain on layer to table conversion when there is only one geometry fieldgetfield(to fix issue #246) which brings 5% performance gain to the latterPerformance results Current master
With Shapefile.jl
Current master with
ogr_f_stealgeometryused ingetcolumnWith new OGR parametric types and with
ogr_f_stealgeometryused ingetcolumn1. ArchGDAL OGR related types moved to parametric composite types
Trying to dispatch on types required:
1.1 Creation of two parametric singleton types for fields and geomfields, and of a featuredefn like type alias
FTypefor fields, which is paramaterized by a tuple ofOGRFieldTypeandOGRFieldSubTypeGTypefor geomfields, which is parameterized by anOGRwkbGeometryTypeFDTypefor featuredefn, which is a 2-tuple of a tuple ofGTypeand a tuple ofFTypeNote 1: I have added convert functions from
FTypeandDataTypewhich yields a more readable correspondance between Julia DataTypes and GDAL Field type ids, than the current separated convert functions fromOGRFieldTypeandOGRFieldSubTypetoDataTypeNote 2: I have also added a commented draft of new
Geometryparametric singleon type (namedGeom) based onGTypebut not used it since I have not identified any benefit1.2 Duplication of most of OGR related composite types and parameterization with
FType,GTypeandFDTypeThe duplicated types have the
FDPprefixFDP_GeomFieldDefnandFDP_FieldDefntypes are parameterized byGTypeandFTypeFDP_FeatureDefn,FDP_FeatureandFDP_FeatureLayertypes are parameterized byFDTypeAll of them have been set as subtypes of corresponding abstract supertypes
DUAL_AbstractXXX, in order to limit the duplication of functions code when not necessary.Note: A plain FeatureDefn parametric composite type could later be used as type parameter for Feature and FeatureLayer
1.3 Modification of a few functions, used to show a layer and convert it to a table
getFDPlayerto handle a feature layer data source with all the newly introduced parametric composite typesFDTypeinformation, have just been extended to acceptDUAL_AbstractXXXFDTypeinformation, have been duplicated as generated functionsNote: In the event of the unlikely use case where there would be too many generated functions (a lot of different data sources with different featuredefn) for compilation to be performed in a reasonable time or even to succeed, the generated functions could be moved to optionally-generated functions. The fallbacks being the legacy functions
1.4 Side additions
Enumtype id for user convenience. Disabled in the PR because it is not convenient when developping using revise.jl because it redefines the related constants. I have not found a way to inhibit the latterI have generalized the
ownedbyfield in feature layer related types which may serve as a basis to fix issue #215, but @yeesian is tackling this one. For all intents and purposes here are below the relationships which are proposed:Ownership relations graph
destroyfunctions forFDP_XXXtypes have been created accordingly2. Steal geometry instead of cloning for the first geometry in features
There appears that
Tables.rows(layer)makes a copy of the layer.Since
ogr_f_stealgeometryis faster than the composition ofogr_f_getgeomfieldrefandogr_g_clone, I have modifiedTables.getcolumnto use astealgeomfunction instead of the currentgetgeomfunction.Unfortunately,
ogr_f_getgeomfieldrefis implemented in GDAL C interface only for the default (first) geometry. Datasources with only one geometry per feature is the general case, but that could be an issue to raise in GDAL.3. Further performance optimizations to investigate, regarding the layer to table conversion performance
FDTypeto avoid callingogr_f_getfieldindexCount the number of calls toEDIT: Done only one extra call toogr_l_getnextfeatureto ensure there is no unnecessary call to ititeratecompared to the number of features when converting a layer to table