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

Make ArrayParser internal and prevent multiple create instance of it #297

Closed
vjik opened this issue Jul 21, 2023 · 21 comments
Closed

Make ArrayParser internal and prevent multiple create instance of it #297

vjik opened this issue Jul 21, 2023 · 21 comments

Comments

@vjik
Copy link
Member

vjik commented Jul 21, 2023

1) Suggest make ArrayParser as internal. Users don't must use this class directly, it'sright? (#297 (comment))

  1. For each type casting of string values created new instance of ArrayParser.

public function phpTypecast(mixed $value): mixed
{
if ($this->dimension > 0) {
if (is_string($value)) {
$value = $this->getArrayParser()->parse($value);
}

It would be good prevent this behavior and create instance of ArrayParser once or make ArrayParser methods static.

Similar case: #303 (comment)

@terabytesoftw
Copy link
Member

terabytesoftw commented Jul 21, 2023

  1. Suggest make ArrayParser as internal. Users don't must use this class directly, it'sright?
  2. For each type casting of string values created new instance of ArrayParser.

public function phpTypecast(mixed $value): mixed
{
if ($this->dimension > 0) {
if (is_string($value)) {
$value = $this->getArrayParser()->parse($value);
}

It would be good prevent this behavior and create instance of ArrayParser once or make ArrayParser methods static.

getArrayParser() return new ArrayParser().

Always returns a new instance.

protected function getArrayParser(): ArrayParser

@vjik
Copy link
Member Author

vjik commented Jul 21, 2023

getArrayParser() return new ArrayParser().

And creates new instance every time.

@terabytesoftw
Copy link
Member

getArrayParser() return new ArrayParser().

And creates new instance every time.

yes, that's not a problem, it just resolves the value, it's not saved anywhere.

@vjik
Copy link
Member Author

vjik commented Jul 21, 2023

yes, that's not a problem, it just resolves the value, it's not saved anywhere.

Why create instance every time?
Many times - for each type casting.

@terabytesoftw
Copy link
Member

terabytesoftw commented Jul 21, 2023

yes, that's not a problem, it just resolves the value, it's not saved anywhere.

Why create instance every time? Many times - for each type casting.

But that's not a problem, as i tell you the object is not saved, it only resolves the value.

@terabytesoftw
Copy link
Member

yes, that's not a problem, it just resolves the value, it's not saved anywhere.

Why create instance every time? Many times - for each type casting.

But that's not a problem, as i tell you the object is not saved, it only resolves the value.

It's like widgets, they always create an object, and resolve its value to a string.

@samdark
Copy link
Member

samdark commented Jul 21, 2023

Should be a good optimization. Worth benchmarking.

@terabytesoftw
Copy link
Member

Btw array parser cannot be internal, Db is an abstraction and you may need it for an extension.

@vjik
Copy link
Member Author

vjik commented Aug 1, 2023

Btw array parser cannot be internal, Db is an abstraction and you may need it for an extension.

ArrayParser used in specific implementation for PostgreSQL. When might it be needed somewhere else?

@terabytesoftw
Copy link
Member

terabytesoftw commented Aug 1, 2023

Simple, if I create my own db-pgsql implementation, or just create the PGSQL NON-PDO implementation, for example.

Everything cannot be within the framework, there will be extensions.

@vjik
Copy link
Member Author

vjik commented Aug 1, 2023

Simple, if I create my own db-pgsql implementation, or just create the PGSQL NON-PDO implementation, for example.

Everything cannot be within the framework, there will be extensions.

It's will be extension of yiisoft/db, not yiisoft/db-pgsql. Array parser is not goal of this package, I don't think that yiisoft/db-pgsql should provide it.

@Tigrov
Copy link
Member

Tigrov commented Aug 1, 2023

The parser can potentially be used for in-project optimization.

For example:

$intArray = $command->setSql('SELECT int_array FROM some_table')->queryScalar();
$parsedArray = (new ArrayParser())->parse($intArray);
$castArray = array_map('intval', $parsedArray);

instead of $castArray = $column->phpTypecast($intArray).

@vjik
Copy link
Member Author

vjik commented Aug 1, 2023

The parser can potentially be used for in-project optimization.

Agree. This is good case.

@smirnov-e
Copy link

can potentially be used for in-project optimization.

It's not optimisation, but the only way to get array from raw responses :(

@Tigrov
Copy link
Member

Tigrov commented Aug 3, 2023

It's not optimisation, but the only way to get array from raw responses :(

There is the second way where the parser is not used directly:

$castArray = $column->phpTypecast($rawIntArray);

But the first way will work almost 2 times faster.

@Tigrov
Copy link
Member

Tigrov commented Aug 4, 2023

Benchmarks to compare current implementation with using property or static class:

https://github.com/Tigrov/yii3-benchmarks/blob/master/src/Benchmark/PgsqlBench.php

Results:

    benchParseIntArrayCurrent...............I4 - Mo367.103μs (±0.61%)
    benchParseIntArrayProperty..............I4 - Mo367.761μs (±0.90%)
    benchParseIntArrayVar...................I4 - Mo369.223μs (±0.29%)
    benchParseIntArrayNew...................I4 - Mo385.663μs (±0.31%)
    benchParseIntArrayStatic................I4 - Mo372.307μs (±0.36%)

@vjik
Copy link
Member Author

vjik commented Aug 4, 2023

Benchmarks to compare current implementation with using property or static class:

Optimization in PHP good work.

Seems, for cases when object use clear methods without side effects we can create object many times and it don't degradate performance.

@smirnov-e
Copy link

@vjik benchParseIntArrayNew is 4.8% slower than benchParseIntArrayProperty. Difference is not big, but noticeable.

@vjik
Copy link
Member Author

vjik commented Aug 4, 2023

@vjik benchParseIntArrayNew is 4.8% slower than benchParseIntArrayProperty. Difference is not big, but noticeable.

On my PC result little other:

image

Difference is so small.

@Tigrov
Copy link
Member

Tigrov commented Aug 8, 2023

Changed ITEMS_COUNT to 1, results a slightly different:

    benchParseIntArrayCurrent...............I4 - Mo0.825μs (±1.67%)
    benchParseIntArrayProperty..............I4 - Mo0.765μs (±1.31%)
    benchParseIntArrayVar...................R5 I4 - Mo0.793μs (±1.83%)
    benchParseIntArrayNew...................R1 I3 - Mo0.790μs (±1.25%)
    benchParseIntArrayStatic................R1 I4 - Mo0.982μs (±1.52%)

Looks like with property it works faster but the benchmark doesn't cover initialization of the property (from constructor, method or cache)

@vjik
Copy link
Member Author

vjik commented Aug 8, 2023

Difference is so small and in different environments results may vary slightly. By logic property and static should be most performance, but in optimization most logic not always is most performance.

@Tigrov Little improve benchmark: Tigrov/yii3-benchmarks#1

My results:

image

Looks like directly use new ((new ArrayHelper())->parse()) is most performance variant.

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

No branches or pull requests

5 participants