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

Add a function Evaluator class with policy based caching capabilities #5253

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from

Conversation

robertbindar
Copy link
Contributor

@robertbindar robertbindar commented Aug 23, 2024

#5215 for context and previous reviews.
This work evolved into something a bit more generic than the tile offsets transparent cache we originally intended to write.
This PR adds:

  • Evaluator class
Evaluator<Policy<Key, Value>, decltype(cb)> eval(cb);
val = eval(key);

// where
Value cb(const Key& key);
  • ImmediateEvaluation policy
    Configures the evaluator to execute cb(key) for any invocation of eval(key).
Evaluator<ImmediateEvaluation<Key, Value>, Callback> eval(cb);
eval(key); // equivalent with cb(key)
eval(key); // equivalent with cb(key)
  • MaxEntriesCache policy
    Configures the evaluator to execute cb(key) and cache the results in a LRU cache
    that cannot hold more than N values. Caching value N+1 triggers cache eviction.
Evaluator<MaxEntriesCache<Key, Value, N>, Callback> eval(cb);
  • MemoryBudgetedCache policy
    Configures the evaluator to execute cb(key) and cache the results in a LRU cache
    with a capacity of M bytes where M is passed during the evaluator construction.
Evaluator<MemoryBudgetedCache<Key, Value>, Callback> eval(cb, SizeFn, memory_budget);
// where `SizeFn` is a user provided function that returns the size of its argument in bytes.

[sc-51496]


TYPE: FEATURE
DESC: Add a function Evaluator class with policy based caching capabilities

@robertbindar robertbindar force-pushed the rbin/ch51491/tile_offsets_cache branch 3 times, most recently from 95dab0c to b986936 Compare August 26, 2024 20:09
Copy link
Contributor

@eric-hughes-tiledb eric-hughes-tiledb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs more documentation in certain places. I didn't notice any defects overall. The tests, however, and not table-driven and thus not as extensive as this code deserves.

tiledb/common/evaluator/evaluator.h Show resolved Hide resolved
Comment on lines +42 to +49
/**
* Abstract class for caching policies.
* This contains most of the logic of the caching mechanism
* except budgeting which is policy specific.
*
* @tparam Key The type of key
* @tparam Value The type of value
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class invariants of the three containers should be documented here in the class header. Because they interact with each other, documenting them one at a time is insufficient to fully describe the class behavior.

tiledb/common/evaluator/evaluator.h Show resolved Hide resolved
tiledb/common/evaluator/evaluator.h Show resolved Hide resolved
tiledb/common/evaluator/evaluator.h Show resolved Hide resolved
tiledb/common/evaluator/evaluator.h Show resolved Hide resolved
/** The maximum amount of bytes currently consumed by this cache */
size_t memory_consumed_;

static constexpr size_t overhead_ = sizeof(return_type);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be better declared before the constructor, where it's used. It also definitely needs documentation.

Comment on lines +329 to +331
throw std::logic_error(
"The memory consumed by this value exceeds the budget of the "
"cache.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throwing if the cache is empty and the object still won't fit feels like incorrect behavior. In a case where there's no room in the cache, an evaluator should still evaluate, even if it can't cache the result.

If it's not going to act like that, documentation to that effect belongs in the class header, since it's otherwise a major surprise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very nice catch and I agree the behavior should be to evaluate but not cache. Logging a warning here seems appropriate?

template <
class Policy,
class F,
typename = std::enable_if_t<std::is_invocable_r_v<
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto on requires

return;
}
}
std::this_thread::sleep_for(std::chrono::milliseconds(100));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this still doing here? Delays make for flaky CI. If it's here merely for expedience, make sure it's documented as a deficiency at the top of the test file.

@bekadavis9 bekadavis9 force-pushed the rbin/ch51491/tile_offsets_cache branch from 9733e59 to 2c93955 Compare October 7, 2024 19:14
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

Successfully merging this pull request may close these issues.

2 participants