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.

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

feat(core): drop support for after-step-hook #3633

Closed
wants to merge 7 commits into from

Conversation

matejcik
Copy link
Contributor

Chipmakers hate her!

Local granny discovers One Weird Trick for saving up to 50 % clock cycles

(less on actual hardware)

so it turns out that in loop.py::run() the hottest of our hot paths, there's this code:

if after_step_hook:
   after_step_hook()

what's after_step_hook, you ask?
well, on Trezor T, it's None. so that's one useless code branch
(and no, this isn't optimized out, we're in Python here)

In emulator, and on T2B1, after_step_hook = display.refresh()
I don't exactly recall why this was there. My suspicion is that with the previous UI toolkit, we relied on immediate mode painting on the TT but it didn't work on the emulator (nor when we later added TS3 that paints into an internal buffer even on real hardware).

Then UI2 came along and we're sticking explicit ui.refresh() calls liberally on all painting paths, presumably because the implicit refresh was failing in some weird cases? Maybe Rust is drawing something without hitting the loop? Maybe the screenshotting thing had a problem? I don't know, hence why this is just a draft for now.

...annnnyway. By completely random chance I was profiling the emulator and noticed that display_refresh() gets called billions of times.

Removing the whole after_step_hook mechanism doesn't seem to have any adverse effects in my (limited) testing so far, and it speeds up device-tests in frozen build by a whooping 50 % (that's in headless mode so no actual screen drawing, mind you. but under the hood the texture updates were happening anyway).

@matejcik matejcik marked this pull request as draft March 21, 2024 12:49
Copy link
Member

@mmilata mmilata left a comment

Choose a reason for hiding this comment

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

I don't remember why refresh was added to the paint path, possibly something to do with rust homescreen d00e87e? Either way if this works on T2B1 hw and speeds up tests then let's merge it.

@matejcik matejcik marked this pull request as ready for review March 22, 2024 10:04
@matejcik
Copy link
Contributor Author

Found another extremely subtle bug that could cause test freeze in some racey cases. If we're extra lucky, it will fix test freezing on hardware.
(loop.close() was incorrectly leaving entries in the _paused queue, and it turns out io.poll() would pick up all events for which an entry exists, even if there are no tasks waiting on that entry. This would cause a race between ... something closing out the workflow task, presumably? I haven't dug deep enough to see why the paused queue becomes empty, but it does, and loop.close() is the only place that explicitly removes things from the paused queue (the main loop in run() will just pop the whole thing out)
...anyway. There's a race between something removing an item paused on USB read, and the handle_session read step coming in to insert itself back to that slot. So an USB packet or two can get silently dropped.)

e36ec20, @mmilata please check my logic

Also added a minor change to debuglink that causes the homescreen-at-start-of-test to always be recorded.

@matejcik
Copy link
Contributor Author

...ahh, right, no, the code is wrong, i'm modifying the thing i'm iterating over.

@mmilata
Copy link
Member

mmilata commented Mar 22, 2024

That's nasty. My intuition for loop.py code is bad but what you say is true and the fix seems correct.

without messing with the "current" entry at all
…ersion

so that it's possible to re-sign translation blobs on a specific commit
for older firmware version
This was necessary for hooking display.refresh() with the old UI toolkit.
With the new one, we explicitly refresh the display after every paint, so
implicit after-step refresh seems no longer necessary.
… are gone

this juuust might fix unexplained freezes on hardware
Copy link

core UI changes device test click test persistence test
T2T1 Model T test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
T2B1 Safe 3 3280 test(screens) main(screens) 2724
T3T1 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
All main(screens)

@matejcik
Copy link
Contributor Author

...of course this kills TR click tests with a thing that I'm pretty sure I dealt with in global layout.

@matejcik
Copy link
Contributor Author

matejcik commented Apr 2, 2024

...of course this kills TR click tests with a thing that I'm pretty sure I dealt with in global layout.

turns out it's its own thing. a legitimate bug in TR button code and/or timer handling, that just happened to have been hidden by the loop being a tiny bit slower

@matejcik matejcik mentioned this pull request Apr 10, 2024
@matejcik
Copy link
Contributor Author

superseded by #3702

@matejcik matejcik closed this Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🔎 Needs review
Development

Successfully merging this pull request may close these issues.

None yet

2 participants