Skip to content

Commit

Permalink
Optimization: createOperations(): avoid database lookups for CRS (typ…
Browse files Browse the repository at this point in the history
…ically PROJ strings) using unknown datums

Fixes #4319

With that improvement, the runtime of the following snippet goes from
25.4 second to 0.9 second:
```c
    for(int i = 0; i < 2000; ++i)
    {
        PJ_CONTEXT *C = proj_context_create();
        PJ *P = proj_create_crs_to_crs(C,
           "+proj=longlat +ellps=GRS80 +towgs84=0,0,0,0,0,0,0 +no_defs",
           "+proj=utm +zone=31 +ellps=GRS80 +units=m +no_defs", NULL);
        proj_destroy(P);
        proj_context_destroy(C);
    }
```
  • Loading branch information
rouault committed Nov 11, 2024
1 parent 4f47232 commit fe0b664
Show file tree
Hide file tree
Showing 3 changed files with 73 additions and 17 deletions.
7 changes: 6 additions & 1 deletion src/iso19111/datum.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1557,9 +1557,14 @@ bool GeodeticReferenceFrame::_isEquivalentTo(
bool GeodeticReferenceFrame::hasEquivalentNameToUsingAlias(
const IdentifiedObject *other,
const io::DatabaseContextPtr &dbContext) const {
if (nameStr() == "unknown" || other->nameStr() == "unknown") {
if (nameStr() == other->nameStr() || nameStr() == "unknown" ||
other->nameStr() == "unknown") {
return true;
}
if (ci_starts_with(nameStr(), UNKNOWN_BASED_ON) ||
ci_starts_with(other->nameStr(), UNKNOWN_BASED_ON)) {
return false;
}
if (dbContext) {
if (!identifiers().empty()) {
const auto &id = identifiers().front();
Expand Down
64 changes: 50 additions & 14 deletions src/iso19111/operation/coordinateoperationfactory.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1720,7 +1720,7 @@ void CoordinateOperationFactory::Private::buildCRSIds(
}
}
}
if (ids.empty()) {
if (ids.empty() && !ci_equal(crs->nameStr(), "unknown")) {
std::vector<io::AuthorityFactory::ObjectType> allowedObjects;
auto geogCRS = dynamic_cast<const crs::GeographicCRS *>(crs.get());
if (geogCRS) {
Expand Down Expand Up @@ -1810,6 +1810,13 @@ CoordinateOperationFactory::Private::findOpsInRegistryDirect(
const auto &authFactory = context.context->getAuthorityFactory();
assert(authFactory);

if ((sourceCRS->identifiers().empty() &&
ci_equal(sourceCRS->nameStr(), "unknown")) ||
(targetCRS->identifiers().empty() &&
ci_equal(targetCRS->nameStr(), "unknown"))) {
return {};
}

#ifdef TRACE_CREATE_OPERATIONS
ENTER_BLOCK("findOpsInRegistryDirect(" + objectAsStr(sourceCRS.get()) +
" --> " + objectAsStr(targetCRS.get()) + ")");
Expand Down Expand Up @@ -1991,21 +1998,39 @@ CoordinateOperationFactory::Private::findsOpsInRegistryWithIntermediate(
objectAsStr(sourceCRS.get()) + " --> " +
objectAsStr(targetCRS.get()) + ")");
#endif

const auto &authFactory = context.context->getAuthorityFactory();
assert(authFactory);

const auto dbContext = authFactory->databaseContext().as_nullable();
const auto geodSrc = dynamic_cast<crs::GeodeticCRS *>(sourceCRS.get());
const auto datumSrc =
geodSrc ? geodSrc->datumNonNull(dbContext).as_nullable() : nullptr;
// Optimization: check if the source/target CRS have no chance to match
// a database entry
if (datumSrc && datumSrc->identifiers().empty() &&
(ci_equal(datumSrc->nameStr(), "unknown") ||
ci_starts_with(datumSrc->nameStr(), UNKNOWN_BASED_ON)))
return {};
if (!geodSrc && dynamic_cast<crs::BoundCRS *>(sourceCRS.get()))
return {};
const auto geodDst = dynamic_cast<crs::GeodeticCRS *>(targetCRS.get());
const auto datumDst =
geodDst ? geodDst->datumNonNull(dbContext).as_nullable() : nullptr;
if (datumDst && datumDst->identifiers().empty() &&
(ci_equal(datumDst->nameStr(), "unknown") ||
ci_starts_with(datumDst->nameStr(), UNKNOWN_BASED_ON)))
return {};
if (!geodDst && dynamic_cast<crs::BoundCRS *>(targetCRS.get()))
return {};

std::list<std::pair<std::string, std::string>> sourceIds;
buildCRSIds(sourceCRS, context, sourceIds);
if (sourceIds.empty()) {
auto geodSrc = dynamic_cast<crs::GeodeticCRS *>(sourceCRS.get());
auto geodDst = dynamic_cast<crs::GeodeticCRS *>(targetCRS.get());
if (geodSrc) {
const std::string originatingAuthSrc =
geodSrc->getOriginatingAuthName();
const auto dbContext = authFactory->databaseContext().as_nullable();
const auto candidatesSrcGeod(findCandidateGeodCRSForDatum(
authFactory, geodSrc, geodSrc->datumNonNull(dbContext)));
authFactory, geodSrc, NN_NO_CHECK(datumSrc)));
std::vector<CoordinateOperationNNPtr> res;
for (const auto &candidateSrcGeod : candidatesSrcGeod) {
// Restrict to using only objects that have the same authority
Expand All @@ -2022,11 +2047,11 @@ CoordinateOperationFactory::Private::findsOpsInRegistryWithIntermediate(
(dynamic_cast<crs::GeographicCRS *>(
candidateSrcGeod.get()) != nullptr))) {
if (geodDst) {
const auto srcDatum =
const auto candidateSrcDatum =
candidateSrcGeod->datumNonNull(dbContext);
const auto dstDatum = geodDst->datumNonNull(dbContext);
const bool sameGeodeticDatum =
isSameGeodeticDatum(srcDatum, dstDatum, dbContext);
const bool sameGeodeticDatum = isSameGeodeticDatum(
candidateSrcDatum, NN_NO_CHECK(datumDst),
dbContext);
if (sameGeodeticDatum) {
continue;
}
Expand Down Expand Up @@ -3078,10 +3103,21 @@ void CoordinateOperationFactory::Private::createOperationsWithDatumPivot(
const auto &authFactory = context.context->getAuthorityFactory();
const auto &dbContext = authFactory->databaseContext();

const auto candidatesSrcGeod(findCandidateGeodCRSForDatum(
authFactory, geodSrc, geodSrc->datumNonNull(dbContext.as_nullable())));
const auto candidatesDstGeod(findCandidateGeodCRSForDatum(
authFactory, geodDst, geodDst->datumNonNull(dbContext.as_nullable())));
const auto srcDatum = geodSrc->datumNonNull(dbContext.as_nullable());
if (srcDatum->identifiers().empty() &&
(ci_equal(srcDatum->nameStr(), "unknown") ||
ci_starts_with(srcDatum->nameStr(), UNKNOWN_BASED_ON)))
return;
const auto dstDatum = geodDst->datumNonNull(dbContext.as_nullable());
if (dstDatum->identifiers().empty() &&
(ci_equal(dstDatum->nameStr(), "unknown") ||
ci_starts_with(dstDatum->nameStr(), UNKNOWN_BASED_ON)))
return;

const auto candidatesSrcGeod(
findCandidateGeodCRSForDatum(authFactory, geodSrc, srcDatum));
const auto candidatesDstGeod(
findCandidateGeodCRSForDatum(authFactory, geodDst, dstDatum));

const bool sourceAndTargetAre3D =
geodSrc->coordinateSystem()->axisList().size() == 3 &&
Expand Down
19 changes: 17 additions & 2 deletions test/unit/test_operationfactory.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -684,10 +684,12 @@ TEST(operation, geogCRS_to_geogCRS_compatible_unknown_celestial_body) {
auto srcCRS = nn_dynamic_pointer_cast<CRS>(objSrc);
auto objDst =
createFromUserInput("+proj=longlat +R=10001 +type=crs", dbContext);
const auto queryCounterBefore = dbContext->getQueryCounter();
auto dstCRS = nn_dynamic_pointer_cast<CRS>(objDst);
auto ctxt = CoordinateOperationContext::create(authFactory, nullptr, 0.0);
auto list = CoordinateOperationFactory::create()->createOperations(
NN_NO_CHECK(srcCRS), NN_NO_CHECK(dstCRS), ctxt);
EXPECT_EQ(dbContext->getQueryCounter(), queryCounterBefore);
ASSERT_EQ(list.size(), 1U);
EXPECT_EQ(list[0]->exportToPROJString(PROJStringFormatter::create().get()),
"+proj=noop");
Expand Down Expand Up @@ -3063,11 +3065,13 @@ TEST(operation, createOperation_boundCRS_identified_by_datum) {
"+ellps=clrk80ign +step +proj=pop +v_3 +step +proj=utm +zone=32 "
"+ellps=clrk80ign");

auto authFactory =
AuthorityFactory::create(DatabaseContext::create(), std::string());
auto dbContext = DatabaseContext::create();
auto authFactory = AuthorityFactory::create(dbContext, std::string());
auto ctxt = CoordinateOperationContext::create(authFactory, nullptr, 0.0);
const auto queryCounterBefore = dbContext->getQueryCounter();
auto list = CoordinateOperationFactory::create()->createOperations(
NN_CHECK_ASSERT(src), NN_CHECK_ASSERT(dest), ctxt);
EXPECT_EQ(dbContext->getQueryCounter(), queryCounterBefore);
ASSERT_EQ(list.size(), 1U);
EXPECT_TRUE(list[0]->isEquivalentTo(op.get()));
}
Expand All @@ -3092,6 +3096,17 @@ TEST(operation, boundCRS_of_clrk_66_geogCRS_to_nad83_geogCRS) {
"+proj=pipeline +step +proj=unitconvert +xy_in=deg +xy_out=rad "
"+step +proj=hgridshift +grids=ntv1_can.dat,conus "
"+step +proj=unitconvert +xy_in=rad +xy_out=deg");

auto dbContext = DatabaseContext::create();
auto authFactory = AuthorityFactory::create(dbContext, std::string());
auto ctxt = CoordinateOperationContext::create(authFactory, nullptr, 0.0);
const auto queryCounterBefore = dbContext->getQueryCounter();
auto list = CoordinateOperationFactory::create()->createOperations(
NN_CHECK_ASSERT(src), NN_CHECK_ASSERT(dest), ctxt);
// Two extra queries related to the conus grid
EXPECT_EQ(dbContext->getQueryCounter(), queryCounterBefore + 2);
ASSERT_EQ(list.size(), 1U);
EXPECT_TRUE(list[0]->isEquivalentTo(op.get()));
}

// ---------------------------------------------------------------------------
Expand Down

0 comments on commit fe0b664

Please sign in to comment.