Allow a more efficient pattern of Pipeline memory allocation #176

tsnoam · 2018-12-30T14:01:19Z

This change allows a performance sensitive tight loop to reuse a
Pipeline object instead of allocating a new one each time.

with_capacity allows to preallocate the internal commands vector
to the specified capacity.
Pipeline.query_clear() clears the internal commands vector from
values after the query (but keeps the allocated capacity).
The unitests show the expected behaviour of query vs query_clear.

This change allows a performance sensitive tight loop to reuse a Pipeline object instead of allocating a new one each time. - `with_capacity` allows to preallocate the internal `commands` vector to the specified capacity. - `Pipeline.query_clear()` clears the internal `commands` vector from values after the query (but keeps the allocated capacity). - The unitests show the expected behaviour of `query` vs `query_clear`.

badboy · 2019-01-05T19:24:14Z

Thanks for the contribution! Just wanted to let you know that it will take a bit more until this gets looked at as I am on vacation. I plan to review it when I'm back in the third week of January.

badboy · 2019-01-15T15:55:39Z

IMO the fact that we don't clear the internal command vector is a bug and we should just do that for query. That removes the need for any query_clear function.

badboy · 2019-01-15T16:01:21Z

That will require &mut self on query and execute on Pipeline, but I think it's worthwhile (but should also be documented clearly)

Marwes · 2019-01-15T16:39:36Z

Having query clear the pipeline seems like really confusing behaviour to me. Just having a separate clear method on the pipeline seems more straightforward.

badboy · 2019-01-15T16:49:53Z

@Marwes Why is that confusing behavior? What use-case do you think of to re-use the filled pipeline?

Marwes · 2019-01-16T00:47:10Z

Why is that confusing behavior?

Query then does more than just querying which makes it confusing if I don't care about reuse.

If I am interested in clearing the pipeline for later reuse I am not going to expect that this is done automatically either so I will need to look up how to do that. A separate clear method would be more discoverable in that case.

What use-case do you think of to re-use the filled pipeline?

A pipeline might be pre-created and used multiple for a commonly used query (incrementing/querying a global counter). Even disregarding that, changing the API to take &mut would prevent usage when only a & is available (which does not necessarily mean that the pipeline is used more than once, it might just be a part of a another object that is shared).

tsnoam · 2019-01-16T08:49:39Z

I agree with @Marwes - that is why I created a new function.
I'm also ok with having an explicit clear() function instead of query_clear(). However, I think that most user would want the combined method, so for better ergonomics, I'd prefer to keep query_clear().

tsnoam · 2019-01-17T16:51:37Z

@badboy
I'd be happy to change my PR according to your decision. So what would it be?

badboy · 2019-01-23T20:03:04Z

sorry for the silence for so long, this got a bit buried.

I am fine with not changing query, but introducing a clear method instead. In addition documenting that query doesn't clear would be needed.

fixes according to code review

tsnoam · 2019-01-24T10:44:01Z

@badboy
I've changed the code as requested.

[ci skip]

badboy

I think we need slight rewording. The pipeline can be reused regardless, it just differs on whether that is with the old commands or just re-using the (then cleared) allocation.

badboy · 2019-01-24T10:49:06Z

src/cmd.rs

+    }
+
+    /// Creates an empty pipeline with pre-allocated capacity.  For consistency with the `cmd`
+    /// api a `pipe` function is provided as alias.


The second part here is not true. The pipe function is only equivalent to new() above.

oopss. my bad. will fix.

badboy · 2019-01-24T10:52:16Z

src/cmd.rs

@@ -480,6 +486,9 @@ impl Pipeline {
    ///     .cmd("GET").arg("key_1")
    ///     .cmd("GET").arg("key_2").query(&con).unwrap();
    /// ```
+    ///
+    /// NOTE: In order to clear (and reuse) the `Pipeline` object after calling `query()`, one must


I think the reuse here is slightly misleading.
Without clear the pipeline can be reused with the existing commands in it, clear gives you a clean pipeline without losing the allocation.

Maybe something like:

NOTE: In order to clear and reuse an empty Pipeline object ...

What do you think?

What about:

NOTE: A Pipeline object may be reused after query() with all the commands as were inserted to them. In order to clear a Pipeline object with minimal memory released/allocated, one should call the clear()method before inserting new commands.

Probably "function" is better than "method".

badboy · 2019-01-24T10:53:49Z

src/cmd.rs

@@ -493,6 +502,12 @@ impl Pipeline {
        )
    }

+    /// Clear the internal data strcture of `Pipeline`, allowing it to be reused.


Same here as before: make it more clear that it's empty afterwards.

@badboy
Would you be kind enough to suggest a phrasing for this?

oh... i see you did. didn't scroll up enough...

What about:

Clear a Pipeline object internal data structure. This allows reusing a Pipeline object as a clear object but with a minimal amount of memory released/allocated.

badboy · 2019-01-24T10:53:56Z

src/cmd.rs

@@ -554,6 +569,9 @@ impl Pipeline {
    /// # let con = client.get_connection().unwrap();
    /// let _ : () = redis::pipe().cmd("PING").query(&con).unwrap();
    /// ```
+    ///
+    /// NOTE: In order to clear (and reuse) the `Pipeline` object after calling `execute()`, one
+    ///       must explicitly call `clear()`.


@badboy
as above... :)

tsnoam · 2019-01-24T12:10:50Z

@badboy
Please see my comments on the review.

badboy · 2019-01-25T17:15:50Z

I don't see any changes since my last review comments.
Did you push?

tsnoam · 2019-01-25T19:03:02Z

@badboy
I replied on your comments (started a discussion), please take a look there.

[ci skip]

tsnoam · 2019-02-01T16:22:34Z

@badboy
I've pushed a commit with the documentation changes as I suggested them on the comments.
I hope you'll find it appropriate.

badboy · 2019-02-17T12:04:24Z

👍

tsnoam · 2019-02-18T09:45:49Z

@badboy
Not to be blunt or anything, but when would it be possible to push a new release (with this merged PR) to crates.io?

badboy · 2019-02-18T09:51:24Z

I plan to have one more round of going through the open PRs and then cut a release, so probably tonight or tomorrow.

* Add `LINSERT` command. (redis-rs#176) * Add `LINSERT` command. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * PR comments. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * changelog Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * PR comments. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * javadoc Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> --------- Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Revert changelog. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * PR comments. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Remove TODO. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * PR comments. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * PR comments + rename `Linsert` to `LInsert`. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * PR comments. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Fix UT. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> --------- Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

badboy self-requested a review January 5, 2019 19:24

Add a clear() function and remove the query_clear() function

a4a3a4b

fixes according to code review

fix a typo in comment

5cb5584

[ci skip]

badboy requested changes Jan 24, 2019

View reviewed changes

Fix documentation per CR

7d669a7

[ci skip]

badboy mentioned this pull request Feb 5, 2019

style: Run cargo fmt #181

Merged

badboy approved these changes Feb 17, 2019

View reviewed changes

badboy merged commit dd00e78 into redis-rs:master Feb 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow a more efficient pattern of Pipeline memory allocation #176

Allow a more efficient pattern of Pipeline memory allocation #176

tsnoam commented Dec 30, 2018

badboy commented Jan 5, 2019

badboy commented Jan 15, 2019

badboy commented Jan 15, 2019

Marwes commented Jan 15, 2019

badboy commented Jan 15, 2019

Marwes commented Jan 16, 2019

tsnoam commented Jan 16, 2019

tsnoam commented Jan 17, 2019

badboy commented Jan 23, 2019

tsnoam commented Jan 24, 2019

badboy left a comment

badboy Jan 24, 2019

tsnoam Jan 24, 2019

badboy Jan 24, 2019

tsnoam Jan 24, 2019

tsnoam Jan 24, 2019

badboy Jan 24, 2019

tsnoam Jan 24, 2019

tsnoam Jan 24, 2019

tsnoam Jan 24, 2019

badboy Jan 24, 2019

tsnoam Jan 24, 2019

tsnoam commented Jan 24, 2019

badboy commented Jan 25, 2019 •

edited

Loading

tsnoam commented Jan 25, 2019

tsnoam commented Feb 1, 2019

badboy commented Feb 17, 2019

tsnoam commented Feb 18, 2019

badboy commented Feb 18, 2019

Allow a more efficient pattern of Pipeline memory allocation #176

Allow a more efficient pattern of Pipeline memory allocation #176

Conversation

tsnoam commented Dec 30, 2018

badboy commented Jan 5, 2019

badboy commented Jan 15, 2019

badboy commented Jan 15, 2019

Marwes commented Jan 15, 2019

badboy commented Jan 15, 2019

Marwes commented Jan 16, 2019

tsnoam commented Jan 16, 2019

tsnoam commented Jan 17, 2019

badboy commented Jan 23, 2019

tsnoam commented Jan 24, 2019

badboy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsnoam commented Jan 24, 2019

badboy commented Jan 25, 2019 • edited Loading

tsnoam commented Jan 25, 2019

tsnoam commented Feb 1, 2019

badboy commented Feb 17, 2019

tsnoam commented Feb 18, 2019

badboy commented Feb 18, 2019

badboy commented Jan 25, 2019 •

edited

Loading