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

Generated hydrator and extractor should be a closure, not a class #64

Open
4 tasks
Ocramius opened this issue Jan 14, 2017 · 4 comments · May be fixed by #78
Open
4 tasks

Generated hydrator and extractor should be a closure, not a class #64

Ocramius opened this issue Jan 14, 2017 · 4 comments · May be fixed by #78
Assignees

Comments

@Ocramius
Copy link
Owner

Ocramius commented Jan 14, 2017

As per discussions in #59, hydrator and extractor shouldn't be composed into a zend-hydrator, but should instead be two closures.

An example API would be:

interface GenerateHydrator 
{
    public function __invoke(string $className) : callable;
}

interface GenerateExtractor 
{
    public function __invoke(string $className) : callable;
}

They would be used as following:

$object = $generateHydrator->__invoke(get_class($object))($object, $data); 
$data = $generateExtractor->__invoke(get_class($object))($object); 

Composing them into a Hydrator instance that follows zend-hydrator spec is simple, but we'd also get a decent performance improvement by just relying on functional composition above.

This would allow us to:

  • gain performance (one less method call per hydrate/extract)
  • remove the need of the visitor pattern to modify an existing class (we just generate closures from scratch)
  • simplify API
  • provide type-safe closures (since we can add parameter and return type hints)
@Ocramius Ocramius added this to the 2.1.0 milestone Jan 14, 2017
@Ocramius Ocramius self-assigned this Jan 14, 2017
@pounard
Copy link
Contributor

pounard commented Jan 14, 2017

I have though of this, here are my ideas.

Nominal use case, no inheritance

class A {
    private $a;
}

Generated closure:

$hydrateA = Closure::bind(function (array $data, A $object) {
  $object->a = $values['a'];
}, null, A::class);

Hydration:

$object = new A();
$data = ['a' => 1];
$hydrateA($data, $object);

Notes:

  • class properties visibility does not matter

Inheritance with visible properties use case

class B {
    protected $b;
}

class A extends B {
    private $a;
}

Generated closure:

$hydrateA = Closure::bind(function (array $data, A $object) {
  $object->b = $values['b'];
  $object->a = $values['a'];
}, null, A::class);

Hydration:

$object = new A();
$data = ['a' => 1, 'b' => 2];
$hydrateA($data, $object);

Notes:

  • only one closure is needed, since properties are visible

Inheritance with hidden properties use case

class B {
    private $b;
}

class A extends B {
    private $a;
}

Generated closure:

$hydrateB = Closure::bind(function (array $data, B $object) {
  $object->b = $values['b'];
}, null, B::class);

$hydrateA = Closure::bind(function (array $data, A $object) use ($hydrateB) {
  $hydrateB->__invoke($data, $object);
  $object->a = $values['a'];
}, null, A::class);

Hydration:

$object = new A();
$data = ['a' => 1, 'b' => 2];
$hydrateA($data, $object);

Notes:

  • closure for A needs to use closure for B
  • caller does not need to know that closure for B exists
  • for closure (re)definition, defining all in the same scope if enough
    for this to work

Deep inheritance with hidden properties use case

class C {
    private $c;
}

class B extends C {
    private $b;
}

class A extends B {
    private $a;
}

Generated closure:

$hydrateC = Closure::bind(function (array $data, C $object) {
  $object->c = $values['c'];
}, null, C::class);

$hydrateB = Closure::bind(function (array $data, B $object) use ($hydrateC) {
  $hydrateC->__invoke($data, $object);
  $object->b = $values['b'];
}, null, B::class);

$hydrateA = Closure::bind(function (array $data, A $object) use ($hydrateB) {
  $hydrateB->__invoke($data, $object);
  $object->a = $values['a'];
}, null, A::class);

Hydration:

$object = new A();
$data = ['a' => 1, 'b' => 2, 'c' => 3];
$hydrateA($data, $object);

Notes:

  • notes are the same as inheritance with hidden properties

Deep inheritance with mixed properties use case

class C {
    private $c;
}

class B extends C {
    protected $b;
}

class A extends B {
    private $a;
}

Generated closure:

$hydrateC = Closure::bind(function (array $data, C $object) {
  $object->c = $values['c'];
}, null, C::class);

$hydrateB = Closure::bind(function (array $data, B $object) use ($hydrateC) {
  $hydrateC->__invoke($data, $object);
  $object->b = $values['b'];
}, null, B::class);

$hydrateA = Closure::bind(function (array $data, A $object) use ($hydrateB) {
  $hydrateB->__invoke($data, $object);
  $object->a = $values['a'];
}, null, A::class);

Or alternate scenario where $hydrateB can be skipped:

$hydrateC = Closure::bind(function (array $data, C $object) {
  $object->c = $values['c'];
}, null, C::class);

$hydrateA = Closure::bind(function (array $data, A $object) use ($hydrateC) {
  $hydrateC->__invoke($data, $object);
  $object->b = $values['b'];
  $object->a = $values['a'];
}, null, A::class);

Hydration:

$object = new A();
$data = ['a' => 1, 'b' => 2];
$hydrateA($data, $object);

Notes:

  • closure for B hydration can be skipped
  • nevertheless, it can be generated as well even if unneeded: in case
    hydrating a B instance is asked, it would be pre-generated

General notes

  • since closures are variables, using a custom namespace is enough for
    avoiding name conflicts
  • all closures can and probably should be in the same namespace
  • all of this can be generated without using an AST, reflexion is enough
  • extractors works the same way

VolCh added a commit to VolCh/GeneratedHydrator that referenced this issue Jun 13, 2018
@VolCh VolCh linked a pull request Jun 13, 2018 that will close this issue
@Ocramius Ocramius removed this from the 3.0.0 milestone Mar 4, 2019
@pounard
Copy link
Contributor

pounard commented Jul 13, 2019

Actually I partially implemented this in a fork of mine. My use case here is to keep backward compatibility and zend-hydrator interface, so in order to achieve this:

  • instead of having an array access to get the extractor and hydrator callbacks, they are set on static properties,
  • static properties are initialized outside of the class, right under it within the same file.

Here is a an exemple:

/**
 * This is a generated hydrator for the MakinaCorpus\Normalizer\Benchmarks\MockTextWithFormat class
 */
final class G402f10984f89322030223b7630346c10ce9bf0a4 implements HydratorInterface
{
    public static $hydrate0;
    public static $extract0;

    public function hydrate(array $data, $object)
    {
        self::$hydrate0->__invoke($object, $data);
        return $object;
    }

    public function extract($object)
    {
        $ret = [];
        self::$extract0->__invoke($object, $ret);
        return $ret;
    }
}

G402f10984f89322030223b7630346c10ce9bf0a4::$hydrate0 = \Closure::bind(static function ($object, $values) {
    if (isset($values['text']) || $object->text !== null && \array_key_exists('text', $values)) {
        $object->text = $values['text'];
    }
    if (isset($values['format']) || $object->format !== null && \array_key_exists('format', $values)) {
        $object->format = $values['format'];
    }
}, null, 'MakinaCorpus\\Normalizer\\Benchmarks\\MockTextWithFormat');

G402f10984f89322030223b7630346c10ce9bf0a4::$extract0 = \Closure::bind(static function ($object, &$values) {
    $values['text'] = $object->text;
    $values['format'] = $object->format;
}, null, 'MakinaCorpus\\Normalizer\\Benchmarks\\MockTextWithFormat');

I still keep and use this fork for two reasons:

  • a branch keeps php 5.6 compatibility (real world if not perfect, and clients don't always want or can upgrade their environments),
  • I don't want to keep all those tricky dependencies (visitor, ast and such).

You can see this code in the master branch of the project: https://github.com/makinacorpus/generated-hydrator/blob/master/src/GeneratedHydrator/ClassGenerator/PHP5HydratorGenerator.php

I intend to move the generated code to PHP7 in order to use ?? or ?: operators, I'm actually investigating this.

@pounard
Copy link
Contributor

pounard commented Jul 13, 2019

Needless to say I would be very happy to reconcile my forked project with this one, so I can help if you wish.

@kelunik
Copy link

kelunik commented Jan 10, 2021

I've written a small PoC based on the ideas here today. https://gist.github.com/kelunik/3548c20696726cee2dbd69631b344557

Without code generation but direct closures instead, it's faster than the current code in my first benchmark.

➜ vendor/bin/phpbench run --filter AllPrivate
PhpBench @git_tag@. Running benchmarks.
Using configuration file: /home/kelunik/GitHub/Ocramius/GeneratedHydrator/phpbench.json

\GeneratedHydratorBenchmark\AllPrivateClassHydrationRefBench

    benchConsume............................I199 [μ Mo]/r: 0.447 0.416 (μs) [μSD μRSD]/r: 0.105μs 23.59%

\GeneratedHydratorBenchmark\LaminasHydrationBench

    benchMethodAllPrivate...................I199 [μ Mo]/r: 38.585 35.965 (μs) [μSD μRSD]/r: 7.311μs 18.95%
    benchReflectionAllPrivate...............I199 [μ Mo]/r: 5.680 5.220 (μs) [μSD μRSD]/r: 1.563μs 27.52%

\GeneratedHydratorBenchmark\AllPrivateClassHydrationBench

    benchConsume............................I199 [μ Mo]/r: 0.579 0.510 (μs) [μSD μRSD]/r: 0.220μs 37.92%

4 subjects, 800 iterations, 400 revs, 0 rejects, 0 failures, 0 warnings
(best [mean mode] worst) = 0.400 [11.323 10.528] 1.080 (μs)
⅀T: 9,058.280μs μSD/r 2.300μs μRSD/r: 26.995%

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

Successfully merging a pull request may close this issue.

3 participants