cpp-lru-cache
cpp-lru-cache copied to clipboard
lamerman
Move value in put()
Hi, While performing some benchmarks I noticed that inserted values are copied, which reduces insert performance. I'll create a pull request to move values into the cache.
@dermojo as I understand after your fix, the values will be copied anyway but during the function call, as it is not reference anymore but value. So you first pass a value and then move that copied value to the list.
@lamerman Sorry, I should have explained my change: You're right, instead of copying the value inside the put function it is taken by-value, so the (potential) copy is moved to the call site of the function, which allows to prevent the copy in some cases, e.g. when the caller moves the value into the function or a temporary value is used.
Consider the following code (for a cache mapping an int to a string):
cache.put(5, "five");
Before the change, a temporary string is created, passed by value, and then copied into the cache. After the function returns, the temporary string is destroyed.
With the changed function, a temporary string is created, passed by value (no copy here), and then moved into the cache. So in this scenario, we're saving an unnecessary copy because the temporary object goes straight into the cache.
The same happens if the caller uses std::move:
cache.put(5, std::move(myValue));
It doesn't make a difference when the caller passes a reference to an existing object, because there still will be a copy, but in case of temporary/move the copy is avoided.
@dermojo, In my view, the interface now looks complicated. User doesn't know that move happens internally unless he looks into the code.
Can we implement a separate method for rvalues that has explicit interface and it's clear what will happen there?
Moreover, a big problem is that for those who already use this library an update will cause additional move constructor involved. If you leave the PR as it is.
I understand, although I think adding another overload for put() that takes an rvalue makes the API more complicated than having a single by-value method (and may introduce ambiguity). Without looking into the function body, you don't see what exactly happens, correct. Looking at the const-ref version, you know that the code will make it copy, because it has to. Taking the input by-value, the user can "hope" that a move is involved, there's no guarantee. But you make it clearer that a copy is made, which is currently somewhat "hidden" in the implementation and can't be prevented.
I'm not sure if I want to invest much time in this change (I don't need it, I just wanted to share a performance improvement). If you're worried about existing users of the code, feel free to reject the PR and leave the code as-is. That's fine with me. But I think the current version is the cleanest from an API point of view.
I agree with you that something should be done in this direction. Currently the only way if your class is heavy-weight is smart pointers. So it should be done. But I cannot agree with the way you implement it in the PR. I need to think about it.