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

Shares the XConnection between all event pools instead of just all event pools on the same thread. #572

Merged
merged 1 commit into from
Jun 18, 2018
Merged

Shares the XConnection between all event pools instead of just all event pools on the same thread. #572

merged 1 commit into from
Jun 18, 2018

Conversation

goddessfreya
Copy link
Contributor

@goddessfreya goddessfreya commented Jun 17, 2018 •
edited
Loading

Shares the XConnection between all event loops instead of just all event
loops on the same thread.

This is needed for adding shared context support to tomaka/glutin, as contexts
must be made with the same native display (and therefore the same
connection.)

Signed-off-by: Hal Gentz [email protected]

  • Tested on all platforms changed
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created an example program if it would help users understand this functionality

@goddessfreya goddessfreya mentioned this pull request Jun 17, 2018
Closed
@francesca64
Copy link
Member

This doesn't look like it segfaults on multithread_window any more often than on master, and I don't see any XCB abort errors/etc.

The segfault seems to happen most often when moving the cursor around, and especially across monitors... anyway, since it's not a regression, I don't expect you to fix that.

francesca64
francesca64 suggested changes Jun 17, 2018
View reviewed changes
CHANGELOG.md Outdated
@@ -14,6 +14,7 @@
- HiDPI support for Wayland.
- `EventsLoop::get_available_monitors` and `EventsLoop::get_primary_monitor` now have identical counterparts on `Window`, so this information can be acquired without an `EventsLoop` borrow.
- `AvailableMonitorsIter` now implements `Debug`.
- In X11, all events loops will now share the same XConnection.
Copy link
Member

Choose a reason for hiding this comment

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

"events loops" -> "event loops" (I know that's the actual type name, but it's a very awkward type name).

The other entries don't use the future tense.

Need backticks around XConnection.

src/platform/linux/mod.rs Outdated
@@ -26,6 +26,8 @@ use self::x11::{XConnection, XError};
use self::x11::ffi::XVisualInfo;
pub use self::x11::XNotSupported;

use parking_lot::Mutex;
Copy link
Member

Choose a reason for hiding this comment

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

This should be on line 9, above the sctk import.

src/platform/linux/mod.rs Outdated
.map_err(|err| err.clone())
})
let xconn = X11_BACKEND.lock();
xconn
Copy link
Member

Choose a reason for hiding this comment

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

You can just call lock() as part of this method chain, which looks cooler.

src/platform/linux/mod.rs Outdated
*xconn.latest_error.lock() = Some(error);
}
});
let xconn = X11_BACKEND.lock();
Copy link
Member

Choose a reason for hiding this comment

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

This variable name should be suffixed with _lock.

@goddessfreya
Shares the XConnection between all event loops instead of just all event
3f91c2d 
loops on the same thread.

This is needed for adding shared context support to glutin, as contexts
must be made with the same native display (and therefore the same
connection.)

Signed-off-by: Hal Gentz <[email protected]>
@francesca64
Copy link
Member

Thanks!

I tested this again, and now my impression is that this does segfault more often than on master. Are you seeing that too?

@goddessfreya
Copy link
Contributor Author

goddessfreya commented Jun 17, 2018 •
edited
Loading

No, I tested with this script:

#!/bin/bash

RUNS=1000

run(){
    f="$(cat <(cargo run 2>&1) | tail -n +2)"
    ret="$(diff <(echo "$f") <(cat suc) | wc -l)"
    if [ $ret -eq "0" ]; then
        echo "g" >> good
        echo g
    else
        echo "b" >> bad
        echo b
    fi
}

touch good
rm good
touch good

touch bad
rm bad
touch bad

N=16
for i in `seq 1 $RUNS`
do
    (
        run
    ) &

    # allow only to execute $N jobs in parallel
    if [[ $(jobs -r -p | wc -l) -gt $N ]]; then
        # wait only for first job
        wait -n
    fi
done
wait

echo GOOD $(wc -l good) VS BAD $(wc -l bad)

Results:

Commit\Thread Number           | 16             | 1
                               | Good   | Bad   | Good    | Bad
upstream/master                | 7      | 993   | 929     | 71
origin/share_xconnection       | 10     | 990   | 901     | 99
Difference                     |  0.3%  | -0.3% | -2%     | 2%

@goddessfreya
Copy link
Contributor Author

goddessfreya commented Jun 17, 2018 •
edited
Loading

Fixed the table, sorry for that, github is weird

@francesca64 francesca64 merged commit 042f5fe into rust-windowing:master Jun 18, 2018
@goddessfreya goddessfreya mentioned this pull request Jul 10, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@francesca64 francesca64 francesca64 approved these changes

Assignees
No one assigned
Labels
DS - x11
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@goddessfreya @francesca64