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

Move value in put() #25

Open
dermojo opened this issue Jul 9, 2018 · 6 comments
Open

Move value in put() #25

dermojo opened this issue Jul 9, 2018 · 6 comments

Comments

@dermojo
Copy link

dermojo commented Jul 9, 2018

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.

@lamerman
Copy link
Owner

@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.

@dermojo
Copy link
Author

dermojo commented Jul 14, 2018

@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.

@lamerman
Copy link
Owner

lamerman commented Jul 17, 2018

@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?

@lamerman
Copy link
Owner

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.

@dermojo
Copy link
Author

dermojo commented Jul 17, 2018

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.

@lamerman
Copy link
Owner

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.

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

2 participants