1452620 - Make reading from WrPipelineInfo more ergonomic

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P3)

Description

[Kartikaya Gupta (email:[e-mail])]

Right now reading a WrPipelineInfo in C++ code means using wr_pipeline_info_next_epoch and wr_pipeline_info_next_removed_pipeline to iterate through the items, which is kinda clunky. We should just copy the structure over and expose it in c++ as an equivalent struct or something. This will also remove the awkward lifetime management where C++ code has to call wr_pipeline_info_delete to free the pointer when it's done with it.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

This also needs documentation to indicate what exactly is being produced in the structure. (See halfway down bug 1449982 comment 30)

Comment 2

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a1a12840fd3711055af1314083d3e170e1e2608

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1452620 - Add an FfiVec structure.

This structure makes it more ergonomic to access vectors from C++. A
Vec<T> can be converted to FfiVec<T> which can safely cross the FFI
boundary. That provides an easy to use read-only access mechanism to the
vector contents. The FfiVec must be returned to rust code to be freed
properly.

Review commit: https://reviewboard.mozilla.org/r/235372/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235372/

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1452620 - Use FfiVec to clean up WrPipelineInfo usage in C++.

Instead of passing a WrPipelineInfo* to C++ code, we can now pass the
WrPipelineInfo directly. C++ code can access the info members without
having to call back into rust code, so the wr_pipeline_info* iterator
methods can be removed. Deleting the structure still requires passing it
back to rust.

Review commit: https://reviewboard.mozilla.org/r/235374/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235374/

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1452620 - Document the WrPipelineInfo structure.

Review commit: https://reviewboard.mozilla.org/r/235376/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235376/

Comment 6

[Aria Beingessner [:Gankra]]

Comment on attachment 8966676 [details]
Bug 1452620 - Add an FfiVec structure.

https://reviewboard.mozilla.org/r/235372/#review241090

just a code-style nit

::: gfx/webrender_bindings/src/bindings.rs:188
(Diff revision 1)
> +impl<T> Drop for FfiVec<T> {
> +    fn drop(&mut self) {
> +        // turn the stuff back into a Vec and let it be freed normally
> +        let _ = unsafe {
> +            Vec::from_raw_parts(
> +                mem::transmute::<*const T, *mut T>(self.data),

transmute is an overpowered laser cannon that should be avoided, use `self.data as *mut T`

Comment 7

[Aria Beingessner [:Gankra]]

Comment on attachment 8966677 [details]
Bug 1452620 - Use FfiVec to clean up WrPipelineInfo usage in C++.

https://reviewboard.mozilla.org/r/235374/#review241118

one style nit

::: gfx/webrender_bindings/src/bindings.rs:659
(Diff revision 1)
>  }
>  
> -#[no_mangle]
> -pub unsafe extern "C" fn wr_pipeline_info_next_epoch(
> -    info: &mut WrPipelineInfo,
> -    out_pipeline: &mut WrPipelineId,
> +impl WrPipelineInfo {
> +    fn new(info: PipelineInfo) -> Self {
> +        WrPipelineInfo {
> +            epochs: FfiVec::from_vec(info.epochs.into_iter().map(|x| x.into()).collect()),

minor style/clarity nit: this *I think* could be .map(WrPipelineEpoch::from)

Comment 8

[Kartikaya Gupta (email:[e-mail])]

(In reply to Alexis Beingessner [:Gankro] from comment #7)
> > +            epochs: FfiVec::from_vec(info.epochs.into_iter().map(|x| x.into()).collect()),
> 
> minor style/clarity nit: this *I think* could be .map(WrPipelineEpoch::from)

For some reason this didn't work with my Into impl. I had to replace the Into impl with the corresponding From impl and then it worked. I thought there was Magic that automatically converted between the two but maybe not?

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8966676 [details]
Bug 1452620 - Add an FfiVec structure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235372/diff/1-2/

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8966677 [details]
Bug 1452620 - Use FfiVec to clean up WrPipelineInfo usage in C++.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235374/diff/1-2/

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8966678 [details]
Bug 1452620 - Document the WrPipelineInfo structure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235376/diff/1-2/

Comment 12

[Nicolas Silva [:nical]]

Comment on attachment 8966677 [details]
Bug 1452620 - Use FfiVec to clean up WrPipelineInfo usage in C++.

https://reviewboard.mozilla.org/r/235374/#review241300

Comment 13

[Nicolas Silva [:nical]]

Comment on attachment 8966678 [details]
Bug 1452620 - Document the WrPipelineInfo structure.

https://reviewboard.mozilla.org/r/235376/#review241304

Comment 14

[Botond Ballo [:botond]]

Comment on attachment 8966678 [details]
Bug 1452620 - Document the WrPipelineInfo structure.

https://reviewboard.mozilla.org/r/235376/#review241566

Thanks!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/62c96f73b6c3
Add an FfiVec structure. r=Gankro
https://hg.mozilla.org/integration/autoland/rev/fc3122cb0725
Use FfiVec to clean up WrPipelineInfo usage in C++. r=Gankro,nical
https://hg.mozilla.org/integration/autoland/rev/cfc40d3357e1
Document the WrPipelineInfo structure. r=botond,nical

Comment 16

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/62c96f73b6c3
https://hg.mozilla.org/mozilla-central/rev/fc3122cb0725
https://hg.mozilla.org/mozilla-central/rev/cfc40d3357e1