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

MSSQLSpatial Fix BCP performance problem (#9112) #9113

Merged
merged 1 commit into from
Jan 21, 2024

Conversation

szekerest
Copy link
Contributor

No description provided.

@coveralls
Copy link
Collaborator

Coverage Status

coverage: 68.754%. remained the same
when pulling 0101453 on szekerest:issue9112
into fc0ec2b on OSGeo:master.

@rouault rouault merged commit 92c4c7a into OSGeo:master Jan 21, 2024
33 checks passed
Comment on lines +1536 to +1537
panSRID = (int *)CPLRealloc(panSRID, sizeof(int) * (nKnownSRID + 1));
papoSRS = (OGRSpatialReference **)CPLRealloc(papoSRS, sizeof(void *) *
Copy link
Contributor

Choose a reason for hiding this comment

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

I know nothing about this driver, and I see that this is the existing implementation.
But two (!) reallocations for each (!) single SRS doesn't look how I would implement such a cache from scratch today. However, maybe it isn't a bottleneck in the DB context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dg0yt What would be the suggested solution? I don't see a performance issue here, especially because we usually look for only a limited set of SRS-s for each data source (a single one in most cases).

Copy link
Member

@rouault rouault Jan 21, 2024

Choose a reason for hiding this comment

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

A std::map<int, std::unique_ptr<OGRSpatialReference>> m_oSRSCache would be the best C++11 replacement for the nKnownSRID, panSRID and papoSRS members.

Copy link
Member

Choose a reason for hiding this comment

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

Rectification this should be a
std::map<int, std::unique_ptr<OGRSpatialReference, OGRSpatialReferenceReleaser>> m_oSRSCache
so that the free function called is OGRSpatialReference::Release() and not plain delete

Copy link
Contributor

Choose a reason for hiding this comment

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

A std::vector<struct> with a reasonable reserve should be fine, thanks to locality.

Copy link
Member

Choose a reason for hiding this comment

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

cf #9119 for a modernization proposal for the PG driver which uses the same pattern

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants