Problem/Motivation

Staring at xhprof output for lots of points, or large geometries, multiple calls were made to GeofieldItem::isEmpty() which is each time creating a geometry object to check if it's empty taking time/memory.

Looking at the code it doesn't need to do this. When the value is set of the field https://git.drupalcode.org/project/geofield/-/blob/8.x-1.x/src/Plugin/Fi... it already creates a geometry and checks if it is empty https://git.drupalcode.org/project/geofield/-/blob/8.x-1.x/src/Plugin/Fi... so the computed values in these columns are NULL if the \Geometry::isEmpty() is TRUE.

Proposed resolution

Don't recreate the Geometry and request if ::isEmpty, but check if one of the columns is NULL.

Remaining tasks

Make an MR.

API changes

None.

Although staring at this I also question if for a field item ::isEmpty https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21TypedData... it is actually correct to return if the geometry that is set is empty. It's a bit like a numeric field returning empty if it contains 0? The value is set, it has value, it is Geometry, it's just Empty Geometry https://trac.osgeo.org/postgis/wiki/DevWikiEmptyGeometry
However, altering that would be a BC break. So just re-using the previous Geometry isEmpty check seemed the better way.

Issue fork geofield-3423627

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

ekes created an issue. See original summary.

ekes opened merge request !27

ekes’s picture

ekes commented
Status: Active » Needs review
ekes’s picture

ekes commented
Status: Needs review » Needs work

Meh. Why did that pass D10.1? In the test setting the $field->value directly doesn't cause the calculation as setValue(). Maybe it should be really.

ekes’s picture

ekes commented

I'd say this is blocked on #3423632: Computed values

itamair made their first commit to this issue’s fork.

itamair’s picture

itamair commented

thanks @ekes for your updates. Going to re-jump and review your last comment asap ...

itamair’s picture

itamair commented
Status: Needs work » Needs review

Soo @ekes ...
I better inspected all this, and I ended up with the conviction that is still nice to define a GeofieldItem::isEmpty() method that not only make sure that the geofield primitive/row (WKT) input is not empty but also that it generates a valid /Geometry.
That is correct, and we should keep it, and also the Test code base that assumes it ...

What we should rather limit, as you correctly mentioned, is the redundant generation of the same /Geometry in the same GeofieldItem class, to limit the time/memory script overhead and better apply the DRY principle.

That's why in my latest commit a preferred to keep basically unchanged the GeofieldItem::isEmpty() method and rather to define a private \Geometry $geometry property that could be generated once and reused also in the GeofieldItem::populateComputedValues() method.

Tests are still passing nicely with this change and $geo_php_wrapper->load($value) is only called once in the class.

I would commit this that looks a nice improvement to me.
Let me know if you disagree, if you see some issues or potential regressions, or evident mistake with this solution of mine.

itamair’s picture

itamair commented
Status: Needs review » Fixed

Well ... I didn't read any feedback to my latest update.
I am feeling my last update / commit is pretty solid, hence I am going to merge this into 8.x-1.x dev, for being part of new upcoming release.

  • itamair committed c8c57ea4 on 8.x-1.x authored by ekes 
    Issue #3423627 by itamair, ekes: isEmpty

  • itamair committed 6ee9a26e on 8.x-1.x 
    Issue #3423627 by itamair, ekes: isEmpty

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.