cc: Keep most recently used resources at the front of resource queue.

cc: Keep most recently used resources at the front of resource queue.

Finding resources in |unused_resources_| from MRU to LRU touches LRU
resources only if needed, which inreases possibility of expiring more
LRU resources within kResourceExpirationDelayMs.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/f5618420f5dc8484d15a5388b45d60011d0d0474
Cr-Commit-Position: refs/heads/master@{#357557}

[prashant.n, 2015-10-30 13:33:18]

Description was changed from

==========
cc: Search MRU to LRU resources from unused resources.

Traverse the |unused_resources_| from rear end as MRU resources are
held at the rear end of the queue. This touches LRU resources only if
needed, which inreases possibility of expiring more LRU resources
within kResourceExpirationDelayMs.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Find resources in unused resources from MRU to LRU.

Traverse the |unused_resources_| from rear end as MRU resources are
held at the rear end of the queue. This touches LRU resources only if
needed, which inreases possibility of expiring more LRU resources
within kResourceExpirationDelayMs.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[prashant.n, 2015-10-30 13:35:31]

prashant.n@samsung.com changed reviewers:
+ danakj@chromium.org, ericrk@chromium.org, reveman@chromium.org,
vmpstr@chromium.org

[prashant.n, 2015-10-30 13:35:31]

PTAL, this currently crashes. I'll fix that soon.

[reveman, 2015-10-30 13:45:30]

The MRU to LRU change makes sense but I'm not sure about all these other changes
in this patch. Can we have a simple patch that does nothing but switch the order
we iterate over unused resources?

[prashant.n, 2015-10-30 19:00:37]

On 2015/10/30 13:45:30, reveman wrote:
> The MRU to LRU change makes sense but I'm not sure about all these other
changes
> in this patch. Can we have a simple patch that does nothing but switch the
order
> we iterate over unused resources?

Other changes are done for moving common code from acquire resource and try
acquire resource to one function.
Do let me know if this okay within same patch or I'll split the patch tomorrow.

[reveman, 2015-10-30 19:33:41]

On 2015/10/30 at 19:00:37, prashant.n wrote:
> On 2015/10/30 13:45:30, reveman wrote:
> > The MRU to LRU change makes sense but I'm not sure about all these other
changes
> > in this patch. Can we have a simple patch that does nothing but switch the
order
> > we iterate over unused resources?
> 
> Other changes are done for moving common code from acquire resource and try
acquire resource to one function.
> Do let me know if this okay within same patch or I'll split the patch
tomorrow.

I would prefer two separate patches.

[prashant.n, 2015-10-31 14:48:24]

PTAL.

[reveman, 2015-11-02 01:47:07]

https://codereview.chromium.org/1420433009/diff/60001/cc/base/scoped_ptr_deque.h
File cc/base/scoped_ptr_deque.h (right):

https://codereview.chromium.org/1420433009/diff/60001/cc/base/scoped_ptr_dequ...
cc/base/scoped_ptr_deque.h:109: scoped_ptr<T> rtake(reverse_iterator position) {
Instead of adding this, can the user of this class be required to do the
reverse_iterator -> iterator conversion? That seems more consistent with the
underlying std::deque api.

https://codereview.chromium.org/1420433009/diff/60001/cc/resources/resource_p...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/1420433009/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.cc:134: Resource*
ResourcePool::TryAcquireResourceWithContentId(uint64_t content_id) {
It shouldn't matter in what order we search the resources when looking for a
specific content id. Can we leave this function unmodified?

https://codereview.chromium.org/1420433009/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.cc:137: // TODO(prashant.n): Move resource finding
logic to common function.
I don't see much duplication of resource finding logic. Is this worthwhile?

[prashant.n, 2015-11-02 04:07:20]

https://codereview.chromium.org/1420433009/diff/60001/cc/base/scoped_ptr_deque.h
File cc/base/scoped_ptr_deque.h (right):

https://codereview.chromium.org/1420433009/diff/60001/cc/base/scoped_ptr_dequ...
cc/base/scoped_ptr_deque.h:109: scoped_ptr<T> rtake(reverse_iterator position) {
I think same thing we'll have to do.

iterator it = --position.base();

and call take(it).

IMO, providing rtake will make sense as it can used with reverse iterator. WDYT?

https://codereview.chromium.org/1420433009/diff/60001/cc/resources/resource_p...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/1420433009/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.cc:134: Resource*
ResourcePool::TryAcquireResourceWithContentId(uint64_t content_id) {
Ahh! I missed one content id = one resource (next_tile_id_). May be in this
patch it will not be needed. When we move the code to common function, there we
need to change this function.

https://codereview.chromium.org/1420433009/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.cc:137: // TODO(prashant.n): Move resource finding
logic to common function.
Yes most of the code in getting resource from unused resources from
AcquireResource and TryAcquireResourceWithContentId is same execpt the search
criteria.

I think we should have separate function to do that job, which I was thining to
put in separate patch.

[prashant.n, 2015-11-02 04:15:50]

typos -
is same execpt the search
is same *except the search

which I was thining to
which I was *thinking to

[reveman, 2015-11-02 04:32:59]

https://codereview.chromium.org/1420433009/diff/60001/cc/base/scoped_ptr_deque.h
File cc/base/scoped_ptr_deque.h (right):

https://codereview.chromium.org/1420433009/diff/60001/cc/base/scoped_ptr_dequ...
cc/base/scoped_ptr_deque.h:109: scoped_ptr<T> rtake(reverse_iterator position) {
On 2015/11/02 at 04:07:20, prashant.n wrote:
> I think same thing we'll have to do.
> 
> iterator it = --position.base();
> 
> and call take(it).
> 
> IMO, providing rtake will make sense as it can used with reverse iterator.
WDYT?

as there's no rerase in std::deque I think it's best to also not have an rtake
in this wrapper class. Letting the user of the container take care of this sgtm.

https://codereview.chromium.org/1420433009/diff/60001/cc/resources/resource_p...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/1420433009/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.cc:137: // TODO(prashant.n): Move resource finding
logic to common function.
On 2015/11/02 at 04:07:20, prashant.n wrote:
> Yes most of the code in getting resource from unused resources from
AcquireResource and TryAcquireResourceWithContentId is same execpt the search
criteria.
> 
> I think we should have separate function to do that job, which I was thining
to put in separate patch.

Ok, feel free to put up a patch for that and I'll review it. No need for this
TODO.

[prashant.n, 2015-11-02 05:28:27]

ptal.

[reveman, 2015-11-02 06:07:52]

https://codereview.chromium.org/1420433009/diff/80001/cc/resources/resource_p...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/1420433009/diff/80001/cc/resources/resource_p...
cc/resources/resource_pool.cc:110:
static_cast<ResourceDeque::iterator>(--rit.base());
if we need this then can we add a base() function to ResourceDeque::iterator
instead of the static_cast?

https://codereview.chromium.org/1420433009/diff/80001/cc/resources/resource_p...
File cc/resources/resource_pool.h (right):

https://codereview.chromium.org/1420433009/diff/80001/cc/resources/resource_p...
cc/resources/resource_pool.h:122: // Holds least recently used resources at the
front of the queue.
why don't we just revert this order and keep the least recently used at the back
instead to avoid the reverse iterator stuff?

[prashant.n, 2015-11-02 07:54:51]

We can do that too. I tried this earlier, but there were many changes than
current change.

[prashant.n, 2015-11-02 07:54:51]

We can do that too. I tried this earlier, but there were many changes than
current change.

[prashant.n, 2015-11-02 17:56:45]

https://codereview.chromium.org/1420433009/diff/80001/cc/resources/resource_p...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/1420433009/diff/80001/cc/resources/resource_p...
cc/resources/resource_pool.cc:110:
static_cast<ResourceDeque::iterator>(--rit.base());
IMO, iterators are different, so typecast would be good, to let user know what
is being done. Otherwise we need to modify scoped ptr deque.
WDYT?

https://codereview.chromium.org/1420433009/diff/80001/cc/resources/resource_p...
File cc/resources/resource_pool.h (right):

https://codereview.chromium.org/1420433009/diff/80001/cc/resources/resource_p...
cc/resources/resource_pool.h:122: // Holds least recently used resources at the
front of the queue.
Should we do this or leave as it is? Do let me know your opinion, so that I can
modify.

[reveman, 2015-11-02 18:16:29]

https://codereview.chromium.org/1420433009/diff/80001/cc/resources/resource_p...
File cc/resources/resource_pool.h (right):

https://codereview.chromium.org/1420433009/diff/80001/cc/resources/resource_p...
cc/resources/resource_pool.h:122: // Holds least recently used resources at the
front of the queue.
On 2015/11/02 at 17:56:45, prashant.n wrote:
> Should we do this or leave as it is? Do let me know your opinion, so that I
can modify.

Unless I missing something, reverting the order is just a matter of changing a
push_back to push_front and a few calls to take_front()/front() to
take_back()/back(). That seems a lot simpler and than the reverse iterator
changes and unless there's something more to it I'd prefer that approach.

[prashant.n, 2015-11-03 05:42:30]

Description was changed from

==========
cc: Find resources in unused resources from MRU to LRU.

Traverse the |unused_resources_| from rear end as MRU resources are
held at the rear end of the queue. This touches LRU resources only if
needed, which inreases possibility of expiring more LRU resources
within kResourceExpirationDelayMs.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Keep most recently used resources at the front of resource queue.

Finding resources in |unused_resources_| from MRU to LRU touches LRU
resources only if needed, which inreases possibility of expiring more
LRU resources within kResourceExpirationDelayMs.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[prashant.n, 2015-11-03 05:46:52]

Changed the order of mru <-> lru. The function CheckBusyResources traverses busy
resources from MRU to LRU, which should have been from LRU to MRU for
consistency, but it would then require rtake kind of functionality. As searching
for removing all possible resources from busy resources in sequence would not
affect, kept its code as it is.

PTAL.

[reveman, 2015-11-03 14:26:49]

lgtm

[prashant.n, 2015-11-03 15:07:56]

The CQ bit was checked by prashant.n@samsung.com

[commit-bot: I haz the power, 2015-11-03 15:08:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1420433009/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1420433009/100001

[commit-bot: I haz the power, 2015-11-03 18:20:35]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-11-03 18:21:29]

Patchset 6 (id:??) landed as
https://crrev.com/f5618420f5dc8484d15a5388b45d60011d0d0474
Cr-Commit-Position: refs/heads/master@{#357557}