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

added bulk read support #42

Closed
wants to merge 8 commits into from
Closed

Conversation

@matej21
Copy link
Contributor

matej21 commented Apr 29, 2016

  • is fallback necessary ?
  • SQLiteStorage and NewMemcachedStorage: read methods internally use bulkRead. Is it ok? I think performance impact won't be serious.
  • non-scalar keys are not allowed in a bulkLoad - alternative would be serialize non-scalar keys. what do you think?
@matej21 matej21 force-pushed the matej21:feature/bulk_load branch from 5ee1b40 to c25b0c1 Apr 29, 2016
* @param callable
* @return array
*/
public function bulkLoad(array $keys, $fallback = NULL)

This comment has been minimized.

Copy link
@Majkl578

Majkl578 Apr 29, 2016

Contributor

Does it make sense to have one fallback? Woulldn't make more sense to have fallback for each key? Or support both?

This comment has been minimized.

Copy link
@matej21

matej21 Apr 29, 2016

Author Contributor

key is passed as a first parameter to the fallback.

This comment has been minimized.

Copy link
@Majkl578

Majkl578 Apr 29, 2016

Contributor

Yes but then you can't easily reuse callback for load and bulkLoad.

This comment has been minimized.

Copy link
@matej21

matej21 Apr 29, 2016

Author Contributor

So each time you call bulkLoad you will build an array of callbacks? that does not sound right.

This comment has been minimized.

Copy link
@Majkl578

Majkl578 Apr 29, 2016

Contributor

What if you want e.g. three values whose value are calculated in a different way? You would have to build this logic by if/switch branching inside the callback. That feels a bit ugly...

This comment has been minimized.

Copy link
@enumag

enumag Apr 29, 2016

Contributor

I agree with @matej21.

@Majkl578 How about this?

public static function createCallbackSwitcher(array $callbacks)
{
    return function ($key, ...$args) use ($callbacks) {
        return $callbacks[$key](...$args);
    };
}

$cache->bulkLoad($keys, self::createCallbackSwitcher($callbacks));
} else {
$cacheData = [];
foreach ($keys as $storageKey => $key) {
$cacheData[$storageKey] = $this->load($key);

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Apr 29, 2016

Contributor

Replace $this->load($key) with $storage->read($storageKey)

@Majkl578

This comment has been minimized.

Copy link
Contributor

Majkl578 commented Apr 29, 2016

I don't think bulk reuse for single operation is a good idea, for performance reasons.

throw new Nette\InvalidArgumentException('Only scalar keys are allowed in a bulkLoad method.');
}
}
$keys = array_combine(array_map([$this, 'generateKey'], $keys), $keys);

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Apr 29, 2016

Contributor

Replaces whole line with

$storageKeys = array_map([$this, 'generateKey'], $keys);
$keys = array_combine(array_map([$this, 'generateKey'], $keys), $keys);
$storage = $this->getStorage();
if ($storage instanceof IBulkReadStorage) {
$cacheData = $storage->bulkRead(array_keys($keys));

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Apr 29, 2016

Contributor

Replace array_keys($keys) with $storageKeys

$cacheData = [];
foreach ($keys as $storageKey => $key) {
$cacheData[$storageKey] = $this->load($key);
}

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Apr 29, 2016

Contributor

Replace whole foreach with

$cacheData = array_combine($keys, array_map([$storage, 'read'], $storageKeys));
@Majkl578

This comment has been minimized.

Copy link
Contributor

Majkl578 commented Apr 29, 2016

Have you seen how doctrine/cache handles this? https://github.com/doctrine/cache/blob/master/lib/Doctrine/Common/Cache/CacheProvider.php
I like that approach more, they implement both, but for drivers that don't support multi-get, they fall back to serial fetch.

* @param string key
* @return array key => value pairs, missing items are omitted
*/
function bulkRead(array $keys);

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Apr 29, 2016

Contributor

TODO: consider returning [$value1, $value2] instead of more complex [$key1 => $value1, $key2 => $value2]

This comment has been minimized.

Copy link
@matej21

matej21 Apr 29, 2016

Author Contributor

Storage would have to maintain the item order and check for missing values. This is easier.

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Apr 29, 2016

Contributor

Yes but the implementation in Cache would be simpler and faster.

This comment has been minimized.

Copy link
@matej21

matej21 Apr 29, 2016

Author Contributor

Yes but the same logic would have to be in the storage itself.

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Apr 29, 2016

Contributor

That would depend on the storage. Now you assume that no storage can implement this efficiently.

This comment has been minimized.

Copy link
@matej21

matej21 Apr 29, 2016

Author Contributor

Yes, I do assume that. Or do you know any storage that doesn't need that logic?

}
$result = [];
foreach ($keys as $storageKey => $key) {
if (isset($cacheData[$storageKey])) {

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Apr 29, 2016

Contributor

Can the condition be replaced with if ($cacheData[$storageKey] === NULL) + drop the else branch?

This comment has been minimized.

Copy link
@matej21

matej21 Apr 29, 2016

Author Contributor

I think it can't. I need to rearrange to the right order, reindex items and fill missing items with null.

@JanTvrdik

This comment has been minimized.

Copy link
Contributor

JanTvrdik commented Apr 29, 2016

The ideas is awesome, but the implementation needs a lot polishing.

require __DIR__ . '/Cache.php';

//storage without bulk load support
test(function () {

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Apr 29, 2016

Contributor

I think dg uses test(function () { // storage without bulk load support pattern

This comment has been minimized.

Copy link
@dg

dg Apr 29, 2016

Member

It does not matter :-)

@matej21

This comment has been minimized.

Copy link
Contributor Author

matej21 commented Apr 29, 2016

btw, the performance impact of bulkRead reuse for a single read is about 10-20%

}
}
$storageKeys = array_map([$this, 'generateKey'], $keys);
$storage = $this->getStorage();

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Apr 29, 2016

Contributor

Drop this line, replace all usages of $storage with $this->storage

if ($storage instanceof IBulkReadStorage) {
$cacheData = $storage->bulkRead($storageKeys);
} else {
$cacheData = array_combine($storageKeys, array_map([$storage, 'read'], $storageKeys));

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Apr 29, 2016

Contributor

Rewrite this to

$result = array_combine($keys, array_map([$storage, 'read'], $storageKeys));

if ($fallback !== NULL) {
    foreach ($result as $key => $value) {
        if ($value === NULL) {
            $result[$key] = $this->save($key, ...);
        }
    }
}

return $result;

This comment has been minimized.

Copy link
@matej21

matej21 Apr 29, 2016

Author Contributor

Like this? I think it is unnecessary code duplication.

matej21 added 5 commits Apr 29, 2016
@matej21

This comment has been minimized.

Copy link
Contributor Author

matej21 commented Apr 29, 2016

updated - storages are optimized

$keys = array_combine(array_map(function ($key) {
return urlencode($this->prefix . $key);
}, $keys), $keys);
$metas = $this->memcached->getMulti(array_keys($keys));

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Apr 29, 2016

Contributor

Replace with

$prefixedKeys = array_map(function ($key) { return urlencode($this->prefix . $key); });
$keys = array_combine($prefixedKeys, $keys); // TODO: can we remove this?
$metas = $this->memcached->getMulti($prefixedKeys);
$stmt->execute(array_merge($keys, [time()]));
$result = [];
$updateSlide = [];
foreach ($stmt->fetchAll(\PDO::FETCH_ASSOC) as $row) {

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Apr 29, 2016

Contributor

Would foreach ($stmt as $row) work as well?

This comment has been minimized.

Copy link
@matej21

matej21 Apr 29, 2016

Author Contributor

isn't iteration affected by ATTR_DEFAULT_FETCH_MODE?

@dg dg force-pushed the nette:master branch 3 times, most recently from 0aed862 to a0c400e Apr 30, 2016
@dg dg force-pushed the nette:master branch from 8c2bba8 to 138b231 May 17, 2016
@dg dg force-pushed the nette:master branch 2 times, most recently from 54c618b to 62a27d9 May 18, 2016
@dg dg closed this in aa11160 Jun 12, 2016
dg added a commit that referenced this pull request Jun 12, 2016
dg added a commit that referenced this pull request Jun 12, 2016
dg added a commit that referenced this pull request Jun 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.