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

PERF: Avoid creating GDALEnv for affine transformations. #2754

Merged
merged 3 commits into from
Feb 8, 2023

Conversation

groutr
Copy link
Contributor

@groutr groutr commented Feb 4, 2023

TransformerBase creates a GDALEnv every time its context is entered. For affine transformations, this is a huge performance hit for nothing.

ref: #2753

Before PR:

In [9]: %%prun
   ...: for lo, la in zip(lons, lats):
   ...:     next(sample.sample_gen(A, [(lo, la)]))
   ...:
         5480066 function calls (5470066 primitive calls) in 9.676 seconds

   Ordered by: internal time

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
    10000    3.796    0.000    4.008    0.000 {method 'start' of 'rasterio._env.GDALEnv' objects}
    10000    1.250    0.000    1.874    0.000 {method 'read' of 'rasterio._io.DatasetReaderBase' objects}
    20000    0.651    0.000    9.596    0.000 sample.py:20(sample_gen)

After PR:

In [8]: %%prun
   ...: for lo, la in zip(lons, lats):
   ...:     next(sample.sample_gen(A, [(lo, la)]))
   ...:
         1910066 function calls (1900066 primitive calls) in 2.968 seconds

   Ordered by: internal time

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
    10000    0.760    0.000    1.286    0.000 {method 'read' of 'rasterio._io.DatasetReaderBase' objects}
    30000    0.275    0.000    0.362    0.000 stride_tricks.py:340(_broadcast_to)
    20000    0.127    0.000    2.916    0.000 sample.py:45(sample_gen)

@groutr
Copy link
Contributor Author

groutr commented Feb 4, 2023

Initially, I thought the best solution was to override __enter__ in AffineTransformer. However, TransformerBase should not be responsible for creating a GDALEnv for its all its subclasses. It should be incumbent upon the class inheriting TransformerBase to know if it needs to create a GDALEnv.

@snowman2
Copy link
Member

snowman2 commented Feb 4, 2023

@groutr thanks for this 👍. Thoughts about creating a GDALTranformerBase class for transformers that require a GDAL environment?

@snowman2 snowman2 changed the title Avoid creating GDALEnv for affine transformations. PERF: Avoid creating GDALEnv for affine transformations. Feb 4, 2023
@snowman2 snowman2 added this to the 1.4.0 milestone Feb 4, 2023
Copy link
Member

@snowman2 snowman2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @groutr 👍

Copy link
Member

@sgillies sgillies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @groutr !

@sgillies sgillies merged commit d04bd00 into rasterio:main Feb 8, 2023
@sgillies
Copy link
Member

sgillies commented Feb 8, 2023

I'm going to cherry pick this one for 1.3.6.

sgillies pushed a commit that referenced this pull request Feb 8, 2023
* Delegate creation of GDALEnv to subclasses that actually require it.

* Create GDALTransformerBase class.

* Fix multiple inheritance issues.
@snowman2 snowman2 modified the milestones: 1.4.0, 1.3.6 Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants