Skip to content

feat: Allow reusing same shared IpcSharedMem for transfers#356

Merged
sagudev merged 2 commits intoservo:mainfrom
sagudev:back-sharedmem
Nov 10, 2024
Merged

feat: Allow reusing same shared IpcSharedMem for transfers#356
sagudev merged 2 commits intoservo:mainfrom
sagudev:back-sharedmem

Conversation

@sagudev
Copy link
Copy Markdown
Member

@sagudev sagudev commented Aug 20, 2024 •
edited
Loading

Simple "solution" to #126, it's not exactly zero-copy, but it allows to modify inner buffer of IpcSharedMem and then send it back. This is all unsafe but we can uphold all safety variants for GPUBuffer (request-response msgs).

Example

before:

const SIZE: usize = 50;
let (tx1, rx1) = ipc::channel().unwrap();
let (tx2, rx2) = ipc::channel().unwrap();
std::thread::spawn(move || {
    let mut data = rx1.recv().unwrap() // allocation 1
                                      .to_vec(); // allocation 2
    for i in 0..=SIZE {
        *data[i] = i as u8;
    }
    tx2.send(IpcSharedMemory::from_bytes(&data)).unwrap(); // allocation 3
});
tx1.send(IpcSharedMemory::from_byte(0, SIZE)).unwrap();
rx2.recv().unwrap();

after:

const SIZE: usize = 50;
let (tx1, rx1) = ipc::channel().unwrap();
let (tx2, rx2) = ipc::channel().unwrap();
std::thread::spawn(move || {
    // SAFETY: This is safe because IpcSharedMemory is constructed in place from main thread (will only be accessed by send)
    let mut ism = unsafe{ rx1.recv().unwrap() }; // allocation 1
    {
        let data = ism.deref_mut();
        for i in 0..=SIZE {
            *data[i] = i as u8;
        }
    }
    tx2.send(ism).unwrap();
});
tx1.send(IpcSharedMemory::from_byte(0, SIZE)).unwrap();
rx2.recv().unwrap();

@sagudev
Copy link
Copy Markdown
Member Author

sagudev commented Aug 20, 2024 

Benchmarking ping_pong_shared_mem_mut_4194304_100: Collecting 100 samples in estimated 5.0000 s (
ping_pong_shared_mem_mut_4194304_100
                        time:   [0.0000 ps 0.0000 ps 0.0000 ps]
Found 13 outliers among 100 measurements (13.00%)
  5 (5.00%) high mild
  8 (8.00%) high severe

Benchmarking ping_pong_shared_mem_4194304_100: Collecting 100 samples in estimated 5.1199 s (200 
ping_pong_shared_mem_4194304_100
                        time:   [225.63 ms 226.61 ms 227.80 ms]
Found 4 outliers among 100 measurements (4.00%)
  1 (1.00%) low mild
  2 (2.00%) high mild
  1 (1.00%) high severe

Benchmarking ping_pong_shared_mem_mut_4194304_125: Collecting 100 samples in estimated 5.0000 s (
ping_pong_shared_mem_mut_4194304_125
                        time:   [0.0192 ps 0.0205 ps 0.0222 ps]
Found 12 outliers among 100 measurements (12.00%)
  4 (4.00%) high mild
  8 (8.00%) high severe

Benchmarking ping_pong_shared_mem_4194304_125: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 5.5s, or reduce sample count to 90.
Benchmarking ping_pong_shared_mem_4194304_125: Collecting 100 samples in estimated 5.5099 s (100 
ping_pong_shared_mem_4194304_125
                        time:   [558.99 ms 562.31 ms 566.19 ms]
Found 7 outliers among 100 measurements (7.00%)
  3 (3.00%) high mild
  4 (4.00%) high severe

@sagudev
Copy link
Copy Markdown
Member Author

sagudev commented Aug 20, 2024

Benches from CI: https://github.com/servo/ipc-channel/actions/runs/10472511882

@sagudev sagudev marked this pull request as ready for review August 20, 2024 13:35
@sagudev sagudev changed the title Allow reusing same shared IpcSharedMem for transfers feat: Allow reusing same shared IpcSharedMem for transfers Aug 20, 2024
@sagudev sagudev requested a review from jdm August 20, 2024 13:57
jschwe
jschwe reviewed Sep 9, 2024
View reviewed changes
jschwe
jschwe reviewed Sep 9, 2024
View reviewed changes
@sagudev sagudev mentioned this pull request Oct 30, 2024
Draft
4 tasks
jdm
jdm approved these changes Oct 31, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@jdm jdm left a comment

Choose a reason for hiding this comment

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

This seems reasonable as a fully-unsafe API. It would be nice if we could detect invalid usage at runtime similar to Arc::get_mut, but I don't have ideas for how to do that soundly.

@sagudev
Copy link
Copy Markdown
Member Author

sagudev commented Nov 10, 2024

This seems reasonable as a fully-unsafe API. It would be nice if we could detect invalid usage at runtime similar to Arc::get_mut, but I don't have ideas for how to do that soundly.

The main problem is we would need to do cross-process locking which is costly, but we might want to implement this in the future to also fulfill other usecases.

@sagudev sagudev added this pull request to the merge queue Nov 10, 2024
Merged via the queue into servo:main with commit aa43418 Nov 10, 2024
@sagudev sagudev mentioned this pull request Mar 3, 2025
Closed
@sagudev sagudev mentioned this pull request Mar 24, 2025
Merged
@sagudev sagudev mentioned this pull request Apr 18, 2025
Closed
github-merge-queue bot pushed a commit to servo/servo that referenced this pull request Apr 23, 2025
@sagudev
Introduce snapshot concept of canvas (#36119)
73b778e 
Each canvas context returns snapshot instead of just raw bytes. This
allows as to hold off conversions (BGRA <-> RGBA, (un)premultiply) to
when/if they are actually needed. For example when loading snapshot into
webgl we can load both RGBA and BGRA so no conversion is really needed.

Currently whole thing is designed to be able to be extend on
servo/ipc-channel#356, to make less copies.
Hence some commented out code.


Fixes #35759
There are tests for these changes in WPT

---------

Signed-off-by: sagudev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jschwe jschwe jschwe left review comments

@jdm jdm jdm approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sagudev @jdm @jschwe