Skip to content

py/objdeque: Expand implementation to be doubly-ended #10724

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

Merged
merged 1 commit into from
Mar 18, 2024

Conversation

LordFlashmeow
Copy link
Contributor

Add pop(), appendleft(), and extend() methods, support iteration and indexing, and initializing from an existing sequence.

Added tests for verifying new behavior.

@github-actions
Copy link

github-actions bot commented Feb 12, 2023

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64: +1128 +0.137% standard[incl +160(data)]
      stm32:  +360 +0.092% PYBV10
     mimxrt:  +344 +0.095% TEENSY40
        rp2:  +344 +0.103% RPI_PICO
       samd:  +356 +0.134% ADAFRUIT_ITSYBITSY_M4_EXPRESS

@codecov-commenter
Copy link

codecov-commenter commented Feb 12, 2023

Codecov Report

Attention: Patch coverage is 98.52941% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 98.39%. Comparing base (cd8eea2) to head (7dff38f).

Files Patch % Lines
py/objdeque.c 98.52% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10724      +/-   ##
==========================================
- Coverage   98.39%   98.39%   -0.01%     
==========================================
  Files         161      161              
  Lines       21079    21140      +61     
==========================================
+ Hits        20740    20800      +60     
- Misses        339      340       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LordFlashmeow LordFlashmeow marked this pull request as ready for review February 12, 2023 21:15
@LordFlashmeow LordFlashmeow force-pushed the deque-expansion branch 3 times, most recently from f2168dd to a235ede Compare February 13, 2023 22:37
@dpgeorge dpgeorge added the py-core Relates to py/ directory in source label Feb 14, 2023
@dpgeorge
Copy link
Member

Thanks for the contribution!

This change does add a lot of code size:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64: +1272 +0.162% standard[incl +160(data)]
      stm32:  +496 +0.126% PYBV10
     cc3200:    +0 +0.000% 
    esp8266:  +552 +0.079% GENERIC
      esp32:  +552 +0.036% GENERIC[incl +112(data)]
     mimxrt:  +528 +0.147% TEENSY40
 renesas-ra:  +496 +0.079% RA6M2_EK
        nrf:    +0 +0.000% pca10040
        rp2:  +512 +0.099% PICO
       samd:  +492 +0.184% ADAFRUIT_ITSYBITSY_M4_EXPRESS

It would be good to try and optimise that down. Then we need to make a decision on whether this is an important feature and worth the cost, and/or whether it should be guarded by new config options.

@LordFlashmeow LordFlashmeow force-pushed the deque-expansion branch 2 times, most recently from c22e1bc to 393926b Compare February 14, 2023 18:36
@LordFlashmeow LordFlashmeow force-pushed the deque-expansion branch 4 times, most recently from 77a439c to 300e053 Compare February 16, 2023 04:18
@dpgeorge
Copy link
Member

Current code size diff:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64: +1112 +0.141% standard[incl +160(data)]
      stm32:  +372 +0.095% PYBV10
     cc3200:    +0 +0.000% 
    esp8266:  +432 +0.062% GENERIC
      esp32:  +416 +0.027% GENERIC[incl +80(data)]
     mimxrt:  +376 +0.106% TEENSY40
 renesas-ra:  +376 +0.060% RA6M2_EK
        nrf:    +0 +0.000% pca10040
        rp2:  +392 +0.121% PICO
       samd:  +360 +0.134% ADAFRUIT_ITSYBITSY_M4_EXPRESS

@andrewleech
Copy link
Contributor

I was thinking perhaps these new features should only be enabled on larger devices, but looking at:

#define MICROPY_PY_COLLECTIONS_DEQUE (MICROPY_CONFIG_ROM_LEVEL_AT_LEAST_EXTRA_FEATURES)

deque is already gated by ROM_LEVEL_AT_LEAST_EXTRA which feels appropriate to me.

@dpgeorge
Copy link
Member

dpgeorge commented Jun 6, 2023

Looking at this again, I did some code-size measurements with various parts in this PR disabled (on stm32 PYBv1.0 which is indicative of most bare-metal ports size):

  • without iteration, without subscript: +228 bytes
  • without iteration: +296 bytes
  • without subscript: +296 bytes
  • the full PR: +364 bytes

I think iteration and subscript are less important/useful than the additional double-ended methods (appendleft and pop) and so can be gated by additional options that are enabled at higher feature levels.

So I'd propose to add the options:

  • MICROPY_PY_COLLECTIONS_DEQUE_ITER
  • MICROPY_PY_COLLECTIONS_DEQUE_SUBSCR

And have these default to MICROPY_CONFIG_ROM_LEVEL_AT_LEAST_FULL_FEATURES.

@andrewleech
Copy link
Contributor

Reducing the default config size by a third sounds reasonable for sure.

Out of interest @LordFlashmeow did you have a particular use-case using iter and indexing?
My personal use of deque is to implement Queue so I don't particularly need these features myself, I'm curious if you are actively using them?

@LordFlashmeow
Copy link
Contributor Author

Out of interest @LordFlashmeow did you have a particular use-case using iter and indexing? My personal use of deque is to implement Queue so I don't particularly need these features myself, I'm curious if you are actively using them?

I’m not actively using them, but supporting iter matches the reference implementation and makes it work with lots of built-in python functions.

Indexing isn’t strictly necessary, but I think it’s natural to have and brings the behavior more in-line with the reference implementation.

@dpgeorge
Copy link
Member

dpgeorge commented Jun 6, 2023

Also, curious what's the use case for .extend(), and related, initialising with an existing iterable? (both added in this PR)

@dpgeorge dpgeorge mentioned this pull request Jan 23, 2024
@SAK917
Copy link

SAK917 commented Feb 13, 2024

In case it helps with this discussion, I have an application for which I need to implement a fast moving average of data acquired from a sensor. Doing this with a performant deque would be ideal, and much cleaner to implement if the deque is iterable. If selecting just a subset of the added functionality LordFlashmeow has implemented is necessary due to space constraints, I would like to log my vote for making deque iterable as that would be very helpful.

@peterhinch
Copy link
Contributor

@SAK917 Have you considered a ringbuf in this application? It has the potential to be more efficient because the storage can be in a fixed-size pre-allocated object such as an array.

@SAK917
Copy link

SAK917 commented Feb 14, 2024

Thanks for your suggestion @peterhinch, I appreciate you taking the time to respond and agree that a ring buffer / circular queue makes sense for my application. That said, my understanding of the MP deque is that it is implemented as a performant statically allocated ring buffer, and as far as that aspect it works well now. However, in addition to this PR bringing the MP deque closer to feature parity with CPython, if the deque was just iterable without the other new functionality, it would make a moving average much easier to implement:

deq = collections.deque((), size)
while true:
    deq.append(sensor.data())
    moving_average = sum(deq) / len(deq)

And I do recognize that there is a moving average workaround for the current non-iterable deque implementation (psuedocode below) that is also more performant than calculating the sum every time the moving average is updated as the example above does:

deq = collections.deque((), size)
sum = 0 
count = 0

while true:
  value = sensor.data()
  deq.append(value)
  sum += value
  count += 1

  if count > size:
    sum -= deq.popleft()
    count -= 1

  moving_average = sum / count

But this is unintuitive and the iterable deque implementation above is considerably clearer and more concise, especially for novice / non-programmers that expect the MP deque to function similarly to the CPython implementation.

@dpgeorge dpgeorge added this to the release-1.23.0 milestone Feb 21, 2024
@projectgus
Copy link
Contributor

This is an automated heads-up that we've just merged a Pull Request
that removes the STATIC macro from MicroPython's C API.

See #13763

A search suggests this PR might apply the STATIC macro to some C code. If it
does, then next time you rebase the PR (or merge from master) then you should
please replace all the STATIC keywords with static.

Although this is an automated message, feel free to @-reply to me directly if
you have any questions about this.

@dpgeorge
Copy link
Member

I have rebased this PR, fixed conflicts, and make STATIC lowercase. I also added the two configuration options for iteration and subscription as mentioned above, although I enabled them by default at the EXTRA features level (same level that collections.deque is enabled) because they don't cost much more (about +70 bytes each on Cortex-M targets).

Current size code change for this PR:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64: +1128 +0.138% standard[incl +160(data)]
      stm32:  +372 +0.096% PYBV10
     cc3200:    +0 +0.000% 
    esp8266:  +452 +0.065% ESP8266_GENERIC
      esp32:  +464 +0.027% ESP32_GENERIC[incl +72(data)]
     mimxrt:  +336 +0.093% TEENSY40
 renesas-ra:  +368 +0.059% EK_RA6M2
        nrf:    +0 +0.000% PCA10040
        rp2:  +320 +0.097% RPI_PICO
       samd:  +344 +0.131% ADAFRUIT_ITSYBITSY_M4_EXPRESS

Add `pop()`, `appendleft()`, and `extend()` methods, support iteration
and indexing, and initializing from an existing sequence.

Iteration and indexing (subscription) have independent configuration flags
to enable them.  They are enabled by default at the same level that
collections.deque is enabled (the extra features level).

Also add tests for checking new behavior.

Signed-off-by: Damien George <damien@micropython.org>
@dpgeorge dpgeorge merged commit 7dff38f into micropython:master Mar 18, 2024
@dpgeorge
Copy link
Member

Now merged. Thanks @LordFlashmeow for the implementation.

@SAK917
Copy link

SAK917 commented Mar 18, 2024

Thank you @LordFlashmeow, this is very helpful!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
py-core Relates to py/ directory in source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants