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

Caching translations implementation #202

Closed
wants to merge 175 commits into from

Conversation

jerinphilip
Copy link
Contributor

@jerinphilip jerinphilip commented Jun 30, 2021

Fixes #201.

src/translator/request.h Outdated Show resolved Hide resolved
@kpu
Copy link
Member

kpu commented Jul 4, 2021

If we don't care too much about redoing some work, why not a direct mapped cache with atomic pointers?

// Limit of size (in bytes) of storage_
size_t storageSizeLimit_;

HashCacheKey hashFn_;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again this is strange, as it holds no state.

Copy link
Contributor Author

@jerinphilip jerinphilip Oct 3, 2021

Choose a reason for hiding this comment

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

This is now a static member function at the interface, with CacheKey and hash(CacheKey) being protected members. Anyone outside is thus forbidden from using these, (Edit: Removed this to avoid confusion).

and the static member function should eliminate any problems with "state".

Copy link
Collaborator

@XapaJIaMnu XapaJIaMnu left a comment

Choose a reason for hiding this comment

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

Might have missed something as this is 1.5k lines of code. Looks better, take a look at the comments.

src/translator/cache.cpp Outdated Show resolved Hide resolved
cacheConfig_(config.sizeInMB * 1024 * 1024, std::chrono::seconds(config.timeToLiveInMilliseconds),
config.removeExpired),
service_(epochManagerConfig_),
context_(service_.GetContext()),
Copy link
Member

@kpu kpu Oct 4, 2021

Choose a reason for hiding this comment

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

I don't understand the lifecycle management of the L4 values you're reading enough. Currently you have one read context_ that is used for all read operations across all threads.

Supposedly context_ keeps things alive:

https://github.com/Microsoft/L4/wiki/Epoch-Queue

Does that mean reads are just accumulating? What is decrementing the reference count?

If they're not just accumulating, then how you can be sure that the value is still there once other threads have done reads against the same context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does that mean reads are just accumulating? What is decrementing the reference count?

This is correct. They are accumulating. Fixes have been pushed, however, L4 is not lock-free on read-anymore in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@XapaJIaMnu

  1. Buckets matter now. test_cache_hparam.sh http://ix.io/3AUP
  2. The multi/single (40/1) cache (on/off) 1M/100K experiment test_cache_overhead.sh http://ix.io/3AUQ

Copy link
Collaborator

@XapaJIaMnu XapaJIaMnu Oct 5, 2021

Choose a reason for hiding this comment

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

I'm confused, were the previous experiments 1thread? This is more so the result I would expect in a multithreaded solution so, a default buckets = num_threads or num_threads/2

Copy link
Contributor Author

@jerinphilip jerinphilip Oct 5, 2021

Choose a reason for hiding this comment

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

were the previous experiments 1thread?

Writes weren't flushed until destruction of Service -> Cache -> L4:, so not a proper contention setting. L4 doesn't allow lock-free reads anymore, limitation of API @ context_.

L4 allows a second reader to accept context and read "multiple" times without locks as demonstrated with multiple key-value pairs in https://github.com/browsermt/L4/blob/master/Examples/main.cpp, while something else writes I suppose. For our use-case, our code is no-longer "lock-free" (be it read or write).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have missed this conversation. Why is the code no-longer lock-free on read? Was what was done before wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

browsermt/L4#2 documents the life-cycle, we had leaks due to read-context being kept alive for lock-free"ness". This would however clear up at the end with accounting not reflecting the same.

I manually added code in L4 to check if the deallocate deferred actions are executed. They are not executed if context_ is held for lock-free reads.

74e890c (#202) is the fix, which should indicate what was done wrong.

@kpu
Copy link
Member

kpu commented Oct 7, 2021

Here's some code for a cache.

  1. It's actually lock free
  2. Manages a shared_ptr without any of the serialization stuff so you can keep the types as is
  3. Downgrades gracefully to bad platforms like webassembly
  4. Doesn't have overengineered options configuration
  5. No new submodule

It probably has a worse eviction policy.

#include <memory>
#include <vector>

template <class Entry, class Hash = std::hash<Entry>, class Equals = std::equal_to<Entry> > class SimpleCache {
  public:
    explicit SimpleCache(std::size_t size) : entries_(size) {}

    template <class Key> std::shared_ptr<Entry> Find(const Key &key) const {
      const std::shared_ptr<Entry> &bucket = entries_[hash_(key) % entries_.size()];
      std::shared_ptr<Entry> ret =
#ifdef WASM
        bucket
#else
        std::atomic_load(&bucket);
#endif
      if (equals_(key, *ret)) {
        return ret;
      } else {
        return std::shared_ptr<Entry>();
      }
    }

    void Store(std::shared_ptr<Entry> entry) {
      std::shared_ptr<Entry> &bucket = entries_[hash_(*entry) % entries_.size()];
#ifdef WASM
      bucket = entry;
#else
      atomic_store(&bucket, entry);
#endif
    }

  private:
    std::vector<std::shared_ptr<Entry> > entries_;

    Hash hash_;
    Equals equals_;
};

@kpu
Copy link
Member

kpu commented Oct 7, 2021

Contiguous buckets will suffer some false sharing. I guess they could be spaced out to cache line size.

@jerinphilip
Copy link
Contributor Author

Closing this in favour of #227.

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.

Caching translations
3 participants