Skip to content

[BREAKING CHANGE] Fixes: #90 Remove idle emulation from asyncio event loop - #541

Merged
penguinolog merged 4 commits into
urwid:masterfrom
penguinolog:idle_emulation_drop
Apr 21, 2023
Merged

[BREAKING CHANGE] Fixes: #90 Remove idle emulation from asyncio event loop#541
penguinolog merged 4 commits into
urwid:masterfrom
penguinolog:idle_emulation_drop

Conversation

@penguinolog

@penguinolog penguinolog commented Apr 19, 2023

Copy link
Copy Markdown
Collaborator

Re-implement abandoned PR #418

  • Fix "not hashable AttrSpec" and it's instance creation price (use __slots__) AttrSpec instances may be created in huge amount, with slots this process consume less resources.
  • Terminal is always created with event loop, if not provided -> SelectEventLoop is used
  • Fixed TornadoEventLoop & AsyncioEventLoop logic (Tornado IOLoop is asyncio based)

For extra details see original PR.

Checklist
  • I've ensured that similar functionality has not already been implemented
  • I've ensured that similar functionality has not earlier been proposed and declined
  • I've branched off the master or python-dual-support branch
  • I've merged fresh upstream into my branch recently
  • I've ran tox successfully in local environment
  • I've included docstrings and/or documentation and/or examples for my code (if this is a new feature)

@penguinolog
[BREAKING CHANGE] Fix: urwid#90 Remove idle emulation from asyncio ev…
2fc2ab1 
…ent loop

Re-implement abandoned PR urwid#418
* Fix "not hashable `AttrSpec`" and it's instance creation price (use `__slots__`)
  `AttrSpec` instances may be created in huge amount,
  with slots this process consume less resources.
* `Terminal` is always created with event loop,
  if not provided -> `SelectEventLoop` is used
* Fixed `TornadoEventLoop` & `AsyncioEventLoop` logic
  (Tornado IOLoop is asyncio based)

For extra details see original PR.
@penguinolog penguinolog changed the title [BREAKING CHANGE] Fix: #90 Remove idle emulation from asyncio event loop [BREAKING CHANGE] Fixes: #90 Remove idle emulation from asyncio event loop Apr 19, 2023
@penguinolog penguinolog linked an issue Apr 19, 2023 that may be closed by this pull request
High CPU load with AsyncioEventLoop #90
Closed
wardi
wardi reviewed Apr 19, 2023
View reviewed changes
Comment thread urwid/display_common.py Outdated

Instance is mutable -> use ID component.
"""
return hash((self.__class__, self._value, id(self)))

@wardi wardi Apr 19, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

if _value is mutable should it be used for computing a __hash__? Won't this cause a problem if it's used as a key in a dict then modified?

@penguinolog penguinolog Apr 19, 2023

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It's made like done by "unsafe hash" in python data classes. Making instance id specific (not recalculated) hash maybe also solution.
Hash is required for terminal example app.

@wardi wardi Apr 19, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

interesting, I didn't know about that. Is it worth mentioning this expected unsafe behavior or the reason for adding __hash__ in the docstring?

@penguinolog penguinolog Apr 19, 2023

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'll add mention in docstring and will recheck repository for mutation calls. If will be possible - will change it to immutable.

@wardi wardi Apr 19, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Immutable AttrSpec would be a good change

@penguinolog penguinolog Apr 19, 2023

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

done

wardi
wardi reviewed Apr 19, 2023
View reviewed changes
Comment thread urwid/event_loop/asyncio_loop.py Outdated
wardi
wardi reviewed Apr 19, 2023
View reviewed changes
Comment thread urwid/event_loop/tornado_loop.py
@wardi

wardi commented Apr 19, 2023

Copy link
Copy Markdown
Collaborator

does this change allow handling multiple input/resize events before the screen is redrawn so that the application can "catch up" quickly?

@penguinolog

penguinolog commented Apr 19, 2023

Copy link
Copy Markdown
Collaborator Author

does this change allow handling multiple input/resize events before the screen is redrawn so that the application can "catch up" quickly?

In my test runs it was quick enough during filesystem browse and no issues on widget replacement.

@wardi

wardi commented Apr 19, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator

A test I used to do was steadily drag the terminal window size so the input would fill up with resize events. If the redraw is happening between each event then after you stop resizing the application will take an arbitrarily long time to catch up and be responsive again. If the redraw happens at the end of handling input events then the application will be almost immediately responsive.

@penguinolog

penguinolog commented Apr 19, 2023

Copy link
Copy Markdown
Collaborator Author

A test I used to do was steadily drag the terminal window size so the input would fill up with resize events. If the redraw is happening between each event then after you stop resizing the application will take an arbitrarily long time to catch up and be responsive again. If the redraw happens at the end then the application will be almost immediately responsive.

For me after ~30 seconds of nonstop resize terminal example worked immediately on AsyncioEventLoop and TornadoEventLoop.

@penguinolog
penguinolog requested a review from wardi April 19, 2023 15:41
@penguinolog penguinolog mentioned this pull request Apr 20, 2023
Closed
1 task
wardi
wardi reviewed Apr 21, 2023
View reviewed changes
Comment thread urwid/display_common.py Outdated
@penguinolog @wardi
Update urwid/display_common.py
eea20ad 
Co-authored-by: Ian Ward <ian@excess.org>
@penguinolog
penguinolog merged commit 350ee5c into urwid:master Apr 21, 2023
@rndusr rndusr mentioned this pull request May 6, 2023
Closed
@danschwarz danschwarz mentioned this pull request Jun 15, 2023
Closed
1 of 3 tasks
lunar-mining added a commit to darkrenaissance/darkfi that referenced this pull request Oct 25, 2023
@lunar-mining
dnet: downgrade urwid version
60e1574 
upgrading urwid to 2.2.0 breaks dnet in two ways:

* ui is not visible unless the terminal is in focus
* the screen is not redrawing correctly- contains artifacts from previous draw.

it may have to do with this breaking change that was added to 2.2.0:

urwid/urwid#541

but i am not sure yet, investigating.
@lunar-mining lunar-mining mentioned this pull request Oct 26, 2023
Closed
parazyd pushed a commit to darkrenaissance/darkfi that referenced this pull request Oct 26, 2023
@lunar-mining @parazyd
dnet: downgrade urwid version
246b7e3 
upgrading urwid to 2.2.0 breaks dnet in two ways:

* ui is not visible unless the terminal is in focus
* the screen is not redrawing correctly- contains artifacts from previous draw.

it may have to do with this breaking change that was added to 2.2.0:

urwid/urwid#541

but i am not sure yet, investigating.
parazyd pushed a commit to darkrenaissance/darkfi that referenced this pull request Oct 26, 2023
@lunar-mining @parazyd
dnet: downgrade urwid version
387897c 
upgrading urwid to 2.2.0 breaks dnet in two ways:

* ui is not visible unless the terminal is in focus
* the screen is not redrawing correctly- contains artifacts from previous draw.

it may have to do with this breaking change that was added to 2.2.0:

urwid/urwid#541

but i am not sure yet, investigating.
@penguinolog
penguinolog deleted the idle_emulation_drop branch January 19, 2024 08:18
@ogayot ogayot mentioned this pull request Mar 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wardi wardi wardi approved these changes
@tonycpsu tonycpsu Awaiting requested review from tonycpsu
@ulidtko ulidtko Awaiting requested review from ulidtko

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

High CPU load with AsyncioEventLoop

2 participants

@penguinolog @wardi