Skip to content

bpo-38891: avoid quadratic item access performance of ShareableList - #18996

Merged
pitrou merged 4 commits into
python:masterfrom
tkren:fix-issue-38891
Apr 19, 2020
Merged

bpo-38891: avoid quadratic item access performance of ShareableList#18996
pitrou merged 4 commits into
python:masterfrom
tkren:fix-issue-38891

Conversation

@tkren

@tkren tkren commented Mar 14, 2020

Copy link
Copy Markdown
Contributor

Avoid linear runtime of ShareableList.__getitem__ and ShareableList.__setitem__ by storing running allocated bytes in ShareableList._allocated_bytes instead of the number of bytes for a particular stored item.

https://bugs.python.org/issue38891

@pitrou pitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for spotting this issue. Just an improvement suggestion below.

Comment thread Lib/multiprocessing/shared_memory.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, please write it in a more readable way. For example:

offset = 0
self._allocated_offsets = [0]
for fmt in _formats:
    offset += self._alignment if fmt[-1] != "s" else int(fmt[:-1])
    self._allocated_offsets.append(offset)

@tkren tkren Apr 18, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pitrou I rather not change the type of self._allocated_bytes to list when sequence is not None. Compare this to the call self._allocated_bytes = struct.unpack_from(...) when sequence is None in line 352, which will create a tuple for self._allocated_bytes.

If you do not like the iterable-expression with the walrus operator as part of the tuple constructor call, we either

  1. construct multiple tuples while looping over _formats (feels clumsy)
  2. or sum over _formats similar to
    self._allocated_bytes = tuple(
         itertools.accumulate(_formats,
                              func=lambda total, fmt: total + (self._alignment if fmt[-1] != "s" else int(fmt[:-1])),
                              initial=0)
     )
  3. or use an inline function like
    def _offsets():
       sum_allocated_bytes = 0
       for fmt in _formats:
          sum_allocated_bytes += self._alignment if fmt[-1] != "s" else int(fmt[:-1])
          yield sum_allocated_bytes
    
    self._allocated_bytes = tuple(offset for offset in _offsets())

Which option do you prefer? (Maybe there is a simpler way that I don't see now?)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or just make it a list in all cases, which is the sanest thing to do IMHO.

@bedevere-bot

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@tkren
tkren requested a review from pitrou April 18, 2020 21:50
@pitrou

pitrou commented Apr 18, 2020

Copy link
Copy Markdown
Member

@tkren Did you forget to push any changes?

@tkren

tkren commented Apr 19, 2020

Copy link
Copy Markdown
Contributor Author

Yes I did, sorry about that %-)

I have made the requested changes; please review again

@bedevere-bot

Copy link
Copy Markdown

Thanks for making the requested changes!

@pitrou: please review the changes made to this pull request.

tkren and others added 4 commits April 19, 2020 16:03
Avoid linear runtime of ShareableList.__getitem__ and
ShareableList.__setitem__ by storing running allocated bytes in
ShareableList._allocated_bytes instead of the number of bytes for
a particular stored item.
Always use a list for storing the sequence of running allocated bytes in
ShareableList._allocated_bytes
@pitrou
pitrou merged commit c8f1715 into python:master Apr 19, 2020
@tkren
tkren deleted the fix-issue-38891 branch April 19, 2020 16:31
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

Successfully merging this pull request may close these issues.

4 participants