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

Remove lock from Context #1732

Closed
wants to merge 61 commits into from
Closed

Remove lock from Context #1732

wants to merge 61 commits into from

Conversation

Pjottos
Copy link

@Pjottos Pjottos commented Jun 13, 2022 •
edited

This PR removes the main lock in Context and changes some parts of the API to accommodate this change, mainly necessary to satisfy the borrow checker.

This change brings the following advantages:

  • No more deadlocks, code that would create a deadlock now gives a compile time error.

  • Since Context is no longer required to be Send + Sync, callback shapes can capture !Send + !Sync types. For example the three-d context.

  • Improves performance by a couple percent.

  • It is no longer possible to use the same Context to draw UI on multiple threads at the same time, which is bound to give incorrect results.

    Closes Consider making egui::Context !Send+!Sync #1399

Apologies for the huge PR, but the bulk of the changes is basically search and replace so I hope it's not too difficult to review.

Pjottos and others added 30 commits June 12, 2022 23:31
@Pjottos
First pass in changing Context API to reduce locking
7936657
@Pjottos
Remove reference to Context inside Response
eb8494b
@Pjottos
Store reference to Context in Ui
9c48fa5
@Pjottos
Fix lifetimes
f512c23
@Pjottos
Add immutable context getter to Ui
bb7312f
@Pjottos
Make theme controls use style_mut
f450317
@Pjottos
Replace calls to Ui::painter with Ui::painter_mut
c782388
@Pjottos
Combine RawResponse and Response, take context as argument
f58631e
@Pjottos
Take Ui as parameter in response methods instead of Context
52f9163
@Pjottos
Add ctx parameter to Painter methods
4881bf8
@Pjottos
Pass context argument to several Painter methods
f561e03
@Pjottos
Replace some response field accesses with getter method invocations
db90c90
@Pjottos
More painter context args
001bbc3
@Pjottos
Fix rect borrow
f999d80
@Pjottos
Add Ui::painter_at back
43b2d03
@Pjottos
Fix some errors
b1b6e4e
@Pjottos
Expose ctx reference in Ui
22cd02d 
This makes it possible to reference different parts of Ui mutably at the same time.
With getter methods the context reference is bound to the self lifetime so it would
be impossible to, for instance, use the painter and context at the same time.
@Pjottos
Take Context as parameter in response methods instead of Ui
572e995
@Pjottos
Replace calls to Ui context getters with field accesses
a3242db
@Pjottos
Fix calls to response methods that require context argument
f15f3ec
@Pjottos
Fix edge cases in ui ctx getter replace
502eb7e
@Pjottos
Fix errors
5e2da3f
@Pjottos
Make fields on Response crate-public to allow constructing it in Context
d49d97a
@Pjottos
Fix creation of Response in Context
3c91f1f
@Pjottos
Fix lifetimes on Prepared structs
ceeb538
@Pjottos
Fix low-hanging borrow errors
d6d2a53
@Pjottos
Expose the painter contained in Ui as a field.
44427dd 
This decouples the Painter reference lifetime from the Ui reference lifetime and thus
allows mutably borrowing the Context at the same time as the Painter.
@Pjottos
Fix more borrow errors
b77cb69
@Pjottos
Change Ui::columns to call a closure for each column.
522f1b1 
It is impossible to have multiple Ui instances alive at the same time
because they contain a mutable reference to the Context, so passing a &mut [Ui]
is not possible anymore.
@Pjottos
Fix borrow error in theme buttons
eaa1813
@Pjottos Pjottos changed the title Reduce locking Remove lock from Context Jun 13, 2022
@Pjottos Pjottos marked this pull request as ready for review June 13, 2022 12:28
@Titaniumtown
Copy link
Contributor

Titaniumtown commented Jun 19, 2022 •
edited

Just merged this into my own fork of egui, so I have a few comments as I just migrated my egui application to base off of this:

  • maybe make a note of the requirement of using memory_mut() instead of simply memory() to gain mutable access to memory
  • make a note of requiring a mutable access to context with things like on_hover_text

If this PR is to be accepted, I would recommend something of a migration guide or some deep documentation of the developer-facing changes so projects can more easily migrate.

But I can notice a performance improvement myself, and about a 1% size decrease.

@emilk
Copy link
Owner

emilk commented Jul 3, 2022

This is a very nice effort, but I am not so happy with the worsening ergonomics. Adding <'_> everywhere is very ugly imho, and many methods now require an extra &mut Context argument.

@emilk emilk mentioned this pull request Jul 21, 2022
Closed
@Titaniumtown
Copy link
Contributor

Will any work be done on this? This was very promising.

@emilk emilk mentioned this pull request Jan 19, 2023
Closed
@emilk
Copy link
Owner

emilk commented Feb 4, 2023

I decided to go this route instead:

Still, thanks so much for all the work in this PR 🙏

@emilk emilk closed this Feb 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consider making egui::Context !Send+!Sync
3 participants
@Pjottos @Titaniumtown @emilk