Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve unmarshall performance #126

Merged
merged 17 commits into from
Sep 11, 2022
Merged

Improve unmarshall performance #126

merged 17 commits into from
Sep 11, 2022

Conversation

bdraco
Copy link
Contributor

@bdraco bdraco commented Aug 3, 2022 •
edited
Loading

This change reduces the overhead of unmarshalling with the
goal of having bleak scanners not overwhelming the system.

See related issue:
hbldh/bleak#236 (comment)

(speed_up_unmarsh) % python3 bench/unmarshall.py
Unmarshalling 1000000 bluetooth rssi messages took 13.497067291998974 seconds
(master) % python3 bench/unmarshall.py
Unmarshalling 1000000 bluetooth rssi messages took 47.7756206660124 seconds

@bdraco bdraco force-pushed the unmarshall_performance branch from 03a6507 to 554c3ae Compare August 4, 2022 22:13
@bdraco bdraco marked this pull request as ready for review August 5, 2022 05:23
@bdraco bdraco marked this pull request as draft August 6, 2022 20:21
@bdraco
Copy link
Contributor Author

bdraco commented Aug 6, 2022

I've been running this in production for a while.

I get more broken pipe errors with this change.

Either there is a bug or its too fast that the data hasn't come over the wire yet, and since the unmarshall code never waits for data to arrive over the wire if its split between multiple reads it increases the chance it will process it too fast and it won't be there yet.

@bdraco
Copy link
Contributor Author

bdraco commented Aug 6, 2022

I think broke resume with the change

@bdraco
Copy link
Contributor Author

bdraco commented Aug 6, 2022

Added a test for resume

@bdraco
Copy link
Contributor Author

bdraco commented Aug 7, 2022

Spent another couple hours testing this and its all working great with bleak

@bdraco bdraco marked this pull request as ready for review August 7, 2022 01:35
@bdraco
Copy link
Contributor Author

bdraco commented Aug 7, 2022

Related previous PR
#62

cc @rjarry in case you have any interest based on your previous PR

@bdraco bdraco mentioned this pull request Aug 31, 2022
Closed
23 tasks
bdraco
bdraco commented Sep 1, 2022
View reviewed changes
dbus_next/_private/unmarshaller.py Show resolved Hide resolved
dbus_next/_private/unmarshaller.py Show resolved Hide resolved
dbus_next/_private/unmarshaller.py Show resolved Hide resolved
@bdraco
Copy link
Contributor Author

bdraco commented Sep 1, 2022

Above comments are for the future. I'm speculating on ways to speed it up a bit more, but anything else is going to be a much smaller improvement and can come in a future PR

@Mariusthvdb Mariusthvdb mentioned this pull request Sep 7, 2022
Closed
acrisci
acrisci reviewed Sep 11, 2022
View reviewed changes
test/test_marshaller.py Outdated Show resolved Hide resolved
@acrisci
Copy link
Member

acrisci commented Sep 11, 2022

You have failing tests. Please rebase against master and run make docker-test to see test failures. I believe CICD build should work now.

acrisci
acrisci requested changes Sep 11, 2022
View reviewed changes
Copy link
Member

@acrisci acrisci left a comment

Choose a reason for hiding this comment

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

Test failures

@bdraco
Improve unmarshall performance
3282eed 
This change reduces the overhead of unmarshalling with the
goal of having bleak scanners not overwhelming the system.

See related issue:
hbldh/bleak#236 (comment)

```
(speed_up_unmarsh) % python3 bench/unmarshall.py
Unmarshalling 1000000 bluetooth rssi messages took 14.397348250000505 seconds
(master) % python3 bench/unmarshall.py
Unmarshalling 1000000 bluetooth rssi messages took 47.7756206660124 seconds
```
@bdraco
blow up on end of message so we raise higher in the stack
bab7e9a
@bdraco
fix resume
efe1dc0
@bdraco
fix resume
a75c95b
@bdraco
add resume test
8cd4970
@bdraco
fix test
4151555
@bdraco
docs
276259f
@bdraco bdraco force-pushed the unmarshall_performance branch from 9591d88 to 276259f Compare September 11, 2022 17:09
bdraco
bdraco commented Sep 11, 2022
View reviewed changes
dbus_next/validators.py Outdated Show resolved Hide resolved
@bdraco
Copy link
Contributor Author

bdraco commented Sep 11, 2022

Still looks good

bdraco@Js-MacBook-Pro python-dbus-next % python3 bench/unmarshall.py 
Unmarshalling 1000000 bluetooth rssi messages took 13.812721209134907 seconds

@acrisci
Copy link
Member

acrisci commented Sep 11, 2022

👍

@acrisci acrisci merged commit ab566e1 into altdesktop:master Sep 11, 2022
@bdraco
Copy link
Contributor Author

bdraco commented Sep 11, 2022

Thanks. I have a few more I can pull out as well that I'll cleanup and send as soon. I'm traveling this week so its going to depend on how ✈️ goes and jet lag

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acrisci acrisci acrisci requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bdraco @acrisci