performance improvements #310

digitalkaoz · 2016-09-21T09:55:11Z

fixed unnecessary object references
avoid unnecessary array_merges
speedup travis builds
speedup tests

digitalkaoz · 2016-09-21T12:48:42Z

@bighappyface are you willing to review this one?

staabm · 2016-09-21T13:57:25Z

src/JsonSchema/Constraints/ObjectConstraint.php

-            return array_key_exists($property, $element) ? $element[$property] : $fallback;
-        } elseif (is_object($element)) {
-            return property_exists($element, $property) ? $element->$property : $fallback;
+        if (is_array($element) && array_key_exists($property, $element) /*$this->checkMode == self::CHECK_MODE_TYPE_CAST*/) {


what do you think of isset instead of array_key_exists which is a lot faster.

this would mean you also need an additional call to array_key_exists in case it does allow caching of NULL values. In cases were most calls would already be catched by isset($element[property]) this will speedup things a lot.

so the end result would render to something like

if (is_array($element)) { if (isset($element[property]) || array_key_exists($property, $element)) { return $element[property]; } } // ... return $fallback;

yeah good point, did it

staabm · 2016-09-21T14:21:47Z

related tweet which shows the measured improvements:
https://twitter.com/digitalkaoz/status/778573654712061952

digitalkaoz · 2016-09-21T14:47:09Z

running it in an isolated environment (a single time) doesnt show much difference...but we are running it in a long running process, and there the gain is huge

https://blackfire.io/profiles/compare/5d8fd94c-bb36-4717-b3ad-c24d87bfbf6b/graph

staabm · 2016-09-21T14:50:34Z

at best you would drop the public blackfire comparison link so we can see exactly whats going on.
see https://blog.blackfire.io/profiles-public-sharing.html

stof · 2016-09-21T16:05:54Z

src/JsonSchema/Constraints/Constraint.php

    {
-        $this->uriRetriever = $uriRetriever;
+        return $this->factory->getCheckMode();


this kind of API tells me that your factory is not really a factory (or not only that)

yeah well, removed those getters, they are not really needed and not part of any interface

stof · 2016-09-21T16:07:11Z

src/JsonSchema/Constraints/ObjectConstraint.php

     */
    public function validateDefinition($element, $objectDefinition = null, JsonPointer $path = null)
    {
+        $constraint = $this->getFactory()->createInstanceFor('undefined');


I would name it $undefinedConstraint to make the code more readable

stof · 2016-09-21T16:10:00Z

src/JsonSchema/Constraints/Constraint.php

-        $this->uriRetriever  = $uriRetriever;
-        $this->factory       = $factory;
-        $this->schemaStorage = $schemaStorage;
+    public function __construct(Factory $factory = null)


you should consider providing a BC layer for the constructor signature. Otherwise, this force to bump to 4.0 already. Supporting the old signature with a deprecation warning (building a factory from the older arguments) would make the upgrade path easier

im slightly against supporting BC...i guess nobody is using custom constraints out there so its all part of the internals...im ok with a BC break here

I'm injecting a few constraints, at least until this gets merged: #308

That said, changing my client code isn't a big deal. When can we expect these PRs to get merged? What are we waiting on?

Tobion · 2016-09-21T21:59:46Z

src/JsonSchema/Constraints/Constraint.php

@@ -72,33 +47,13 @@ public function getUriRetriever()
    public function getFactory()
    {
        if (!$this->factory) {
-            $this->factory = new Factory($this->getSchemaStorage(), $this->getUriRetriever(), $this->checkMode);
+            $this->factory = new Factory();


If the factory is now always created in the constructor, there is no point in doing if (!$this->factory) in the public getFactory method anymore.

yeah not needed, fixed

bighappyface · 2016-10-09T19:37:38Z

Hey @shmax I have merged #308

@digitalkaoz would you please rebase this PR on master and resolve conflicts. I will approve/merge/bump once all is green.

digitalkaoz · 2016-10-09T20:13:03Z

@bighappyface ok rebased, should be all green in a few minutes!

bighappyface · 2016-10-09T20:28:03Z

tests/Constraints/CoerciveTest.php

+    /**
+     * @dataProvider getInvalidCoerceTests
+     */
+    public function testInvalidCoerceCases($input, $schema, $errors = [])


array for 5.3

shmax · 2016-10-09T21:34:54Z

tests/Constraints/CoerciveTest.php

+        $validator = new Validator(new Factory($schemaStorage, null, $checkMode));
+        $validator->check(json_decode($input), $schema);
+
+        if ([] !== $errors) {


This one, too...

shmax · 2016-10-09T21:35:19Z

tests/Constraints/CoerciveTest.php

+        $validator = new Validator(new Factory($schemaStorage, null, $checkMode));
+        $validator->check(json_decode($input, true), $schema);
+
+        if ([] !== $errors) {


one more...

who is using 5.3 nowadays? anyway, fixed both ;)

@digitalkaoz Composer still supports PHP 5.3 users today, and it uses this library

@stof is there a plan/policy for Composer to adhere to PHP supported version lifespans?

digitalkaoz · 2016-10-10T07:19:58Z

@shmax im getting some weird errors in my builds (which werent there before your merged PR) any idea where Undefined property: JsonSchema\Constraints\UndefinedConstraint::$schemaStorage comes from? cant find it anywhere in the code

staabm · 2016-10-10T07:26:35Z

@digitalkaoz per travis the stack for this warning is

1) JsonSchema\Tests\Constraints\AdditionalPropertiesTest::testInvalidCases with data set #0 ('{\n                  "prop":"1...     }', '{\n                  "type":"o...     }', null, array(array('', '', 'The property additionalProp i...erties', 'additionalProp')))

Undefined property: JsonSchema\Constraints\UndefinedConstraint::$schemaStorage

/home/travis/build/justinrainbow/json-schema/src/JsonSchema/Constraints/UndefinedConstraint.php:63
/home/travis/build/justinrainbow/json-schema/src/JsonSchema/Constraints/UndefinedConstraint.php:41
/home/travis/build/justinrainbow/json-schema/src/JsonSchema/Constraints/Constraint.php:181
/home/travis/build/justinrainbow/json-schema/src/JsonSchema/Constraints/SchemaConstraint.php:30
/home/travis/build/justinrainbow/json-schema/src/JsonSchema/Validator.php:36
/home/travis/build/justinrainbow/json-schema/tests/Constraints/BaseTestCase.php:41

staabm · 2016-10-10T07:28:42Z

there you go:
https://github.com/justinrainbow/json-schema/blob/master/src/JsonSchema/Constraints/UndefinedConstraint.php#L63

digitalkaoz · 2016-10-10T07:53:46Z

@staabm thanks. i had to rebase once more, should be fixed now

shmax · 2016-10-10T08:09:44Z

getting some weird errors in my builds

Ah, sorry about that--snuck another PR in there ahead of you. I don't have any more surprises, so happy merging...

digitalkaoz force-pushed the master branch 2 times, most recently from 4f96ab3 to 0a48d3a Compare September 21, 2016 10:40

digitalkaoz changed the title ~~avoid unnessary array_merge calls~~ performance improvements Sep 21, 2016

digitalkaoz force-pushed the master branch 7 times, most recently from c1cd87c to e428b84 Compare September 21, 2016 12:26

digitalkaoz force-pushed the master branch 7 times, most recently from 3ad7f54 to 0c45504 Compare September 21, 2016 13:56

staabm reviewed Sep 21, 2016

View reviewed changes

digitalkaoz force-pushed the master branch 3 times, most recently from b67d90e to ad93364 Compare September 21, 2016 14:17

digitalkaoz force-pushed the master branch from ad93364 to b9eed51 Compare September 21, 2016 15:13

stof suggested changes Sep 21, 2016

View reviewed changes

digitalkaoz force-pushed the master branch from b9eed51 to 602f46e Compare September 21, 2016 18:12

Tobion suggested changes Sep 21, 2016

View reviewed changes

digitalkaoz force-pushed the master branch from 602f46e to 5cd62f7 Compare September 22, 2016 07:05

bighappyface added the Needs rebase on target branch label Oct 9, 2016

digitalkaoz force-pushed the master branch from 5cd62f7 to 543bedc Compare October 9, 2016 20:12

bighappyface reviewed Oct 9, 2016

View reviewed changes

bighappyface added Builds Failing and removed Needs rebase on target branch labels Oct 9, 2016

digitalkaoz force-pushed the master branch from 543bedc to 87300bc Compare October 9, 2016 20:59

shmax reviewed Oct 9, 2016

View reviewed changes

digitalkaoz force-pushed the master branch 2 times, most recently from fab3d3c to f452047 Compare October 10, 2016 06:41

performance improvements

c3cde06

digitalkaoz force-pushed the master branch from f452047 to c3cde06 Compare October 10, 2016 07:52

bighappyface removed the Builds Failing label Oct 10, 2016

bighappyface merged commit 31b2954 into jsonrainbow:master Oct 10, 2016

shmax mentioned this pull request Feb 21, 2017

Add option to apply default values from the schema #349

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

performance improvements #310

performance improvements #310

digitalkaoz commented Sep 21, 2016 •

edited

Loading

digitalkaoz commented Sep 21, 2016

staabm Sep 21, 2016

digitalkaoz Sep 21, 2016

staabm commented Sep 21, 2016

digitalkaoz commented Sep 21, 2016 •

edited

Loading

staabm commented Sep 21, 2016 •

edited

Loading

stof Sep 21, 2016

digitalkaoz Sep 21, 2016

stof Sep 21, 2016

digitalkaoz Sep 21, 2016

stof Sep 21, 2016

digitalkaoz Sep 21, 2016

shmax Oct 3, 2016

Tobion Sep 21, 2016

digitalkaoz Sep 22, 2016

bighappyface commented Oct 9, 2016

digitalkaoz commented Oct 9, 2016

bighappyface Oct 9, 2016

digitalkaoz Oct 9, 2016

shmax Oct 9, 2016

shmax Oct 9, 2016

digitalkaoz Oct 10, 2016

stof Oct 10, 2016

bighappyface Oct 10, 2016

digitalkaoz commented Oct 10, 2016

staabm commented Oct 10, 2016

staabm commented Oct 10, 2016

digitalkaoz commented Oct 10, 2016

shmax commented Oct 10, 2016

performance improvements #310

performance improvements #310

Conversation

digitalkaoz commented Sep 21, 2016 • edited Loading

digitalkaoz commented Sep 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staabm commented Sep 21, 2016

digitalkaoz commented Sep 21, 2016 • edited Loading

staabm commented Sep 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bighappyface commented Oct 9, 2016

digitalkaoz commented Oct 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

digitalkaoz commented Oct 10, 2016

staabm commented Oct 10, 2016

staabm commented Oct 10, 2016

digitalkaoz commented Oct 10, 2016

shmax commented Oct 10, 2016

digitalkaoz commented Sep 21, 2016 •

edited

Loading

digitalkaoz commented Sep 21, 2016 •

edited

Loading

staabm commented Sep 21, 2016 •

edited

Loading