Replace SimpleThrottle self.history default value with deque

[JasperSui]

Description

To decrease the time complexity from O(N) to O(1) while adding an element at the first position of the list (self.history), we should use collections.deque with .appendleft() for 10x performance.

Demo:

In [1]: lst = [0]*1000
In [2]: timeit -n1000 lst.insert(0, 1)
1000 loops, best of 3: 794 ns per loop

In [3]: from collections import deque
In [4]: deq = deque([0]*1000)
In [5]: timeit -n1000 deq.appendleft(1)
1000 loops, best of 3: 73 ns per loop

[akx]

Will constructing a deque() only to be thrown away when hitting the cache be a problem?

[JasperSui]

Hi @akx,

Will constructing a deque() only to be thrown away when hitting the cache be a problem?

No, the initialization way is the same as the empty list version. As you can see, there is no other way to initialize self.history but Line 124. What we only modify is the way to maintain a FILO array (self.history) by using collections.deque which is the most suitable for the scenario.

Since we change the data structure of SimpleThrottleRate that might be the most common throttle class, we must clarify it in the changelog to avoid the upgrade defect.

[akx]

My point was mostly that is it e.g. slower to initialize a deque than an empty list? Should this explicitly be

Suggested change

	self.history = self.cache.get(self.key, deque())
	self.history = self.cache.get(self.key)
	if self.history is None:
	self.history = deque()

?

[JasperSui]

I just check the cpython/listobject.c and cpython/Modules/_collectionsmodule.c, and django-redis/cache.py since django/core/cache/backend/base.py wants us to implement .get() ourselves.

The initialization performance difference between list and deque is close I think, but seems like we will declare a deque() in every allow_request() no matter the key exists in cache or not, so I think the explicit way like yours will be better than the original version.

[JasperSui]

A simple benchmark:

In [1]: timeit.timeit("[]", number=10000)
Out[1]: 0.00045701999999891996

In [2]: timeit.timeit("deque()", setup="from collections import deque", number=10000)
Out[2]: 0.000674222999975882

[jaap3]

Does there need to be a some sort of check that the returned value from the cache is really a deque instance? If the cache is populated by a previous version of DRF self.history will still be a list instance right?

[jaap3]

Some cache backends for Django (e.g. Redis) support JSON serialization of cache data (instead of pickle). These might be convert a deque to list (or throw a TypeError on insertion).

[tomchristie]

I'm pretty sure we've seen a similar pull request, that we merged, and then had to revert.
Even tiny changes like this carry a risk factor and are likely to end up breaking someone's codebase, somewhere.
At this point in the project's lifespan it's not worth the trade-off.

Ah... found it: https://github.com/encode/django-rest-framework/blob/f83620dcc9e87f81ddc846c56f2ad87c2e548f8d/docs/community/release-notes.md#3124

[tomchristie]

This: #7849

Which we needed to revert: #7870

[tomchristie]

The most relevant improvement here would be anything that we can do to help guide contributors away from code changing pull requests at this point, unless absolutely neccessary.

[JasperSui]

I'm pretty sure we've seen a similar pull request, that we merged, and then had to revert.
Even tiny changes like this carry a risk factor and are likely to end up breaking someone's codebase, somewhere.
At this point in the project's lifespan it's not worth the trade-off.

Ah... found it: https://github.com/encode/django-rest-framework/blob/f83620dcc9e87f81ddc846c56f2ad87c2e548f8d/docs/community/release-notes.md#3124

Yes, it seemed that some tiny changes will hurt someone unexpectedly, I totally agree with your point.
So maybe we can implement the deque version to be hurtless to others.

Thank you for your time!