1418202 - Serialize ProgramBinary to/from blob/disk

Status: RESOLVED FIXED

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

Description

[Sotaro Ikeda [:sotaro]]

This bug handles ProgramBinary to/from blob/disk.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Can we just have the blobs generated at compile time and have them included in the binary? That lets us avoid any tricky management of a disk cache.

Comment 2

[Sotaro Ikeda [:sotaro]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #1)
> Can we just have the blobs generated at compile time and have them included
> in the binary? That lets us avoid any tricky management of a disk cache.

Yea, I also have same idea:) It seems possible, since we always use d3d11 device.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

I've filed https://github.com/servo/webrender/issues/2580 about doing shader processing at build time.

Comment 4

[Sotaro Ikeda [:sotaro]]

From https://github.com/servo/webrender/issues/2580, it seems better to implement saving ProgramBinary to disc.

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: temporal patch - Add log to measure shader creation time

Comment 19

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8971527 [details] [diff] [review]
wip patch

:jrmuizel, can you feedback to the patch? Though it is still wip.

Comment 20

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8971527 [details] [diff] [review]
wip patch

Review of attachment 8971527 [details] [diff] [review]:
-----------------------------------------------------------------

I took a quick look at this and seems pretty reasonable to me. Can you give an overview of what's happening on what thread when?

Comment 21

[Sotaro Ikeda [:sotaro]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #20)
> 
> I took a quick look at this and seems pretty reasonable to me. Can you give
> an overview of what's happening on what thread when?

I am going to create a diagram to show the overview.

Comment 23

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Comment 25

[Sotaro Ikeda [:sotaro]]

Attachment: diagram of current implementation

Comment 26

[Sotaro Ikeda [:sotaro]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #20)
> Comment on attachment 8971527 [details] [diff] [review]
> wip patch
> 
> Review of attachment 8971527 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I took a quick look at this and seems pretty reasonable to me. Can you give
> an overview of what's happening on what thread when?

attachment 8975772 [details] shows overview.

- Program binary disk cache is loaded on RenderThread when RenderThread is created on GPU process.
- Program binary is saved to disk on worker thread. The task is posted from RenderThread to worker thread.
- Program binary disk cache directory is deleted on main thread in chrome process during gfxPlatform::Init() when it is necessary.

Comment 27

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Comment 28

[Jeff Muizelaar [:jrmuizel]]

The diagram explains things well and the design looks good.

Comment 29

[Sotaro Ikeda [:sotaro]]

Thanks for checking!

Comment 30

[Sotaro Ikeda [:sotaro]]

Attachment: diagram of current implementation

Update a bit.

Comment 31

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Removed unused code.

Comment 32

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Comment 33

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

Please get this patch finalized and reviewed by somebody as it's blocking WR updates now. Or even better would be to write a minimal patch that will allow the WR update to build and pass tests, even if it doesn't have all the gecko-side stuff hooked up.

Note also that (I think) Jeff is on parental leave from this morning, so probably won't be around for the next month or so.

 0:19.73 error[E0061]: this function takes 1 parameter but 0 parameters were supplied
 0:19.73    --> gfx/webrender_bindings/src/bindings.rs:834:25
 0:19.73     |
 0:19.73 834 |     let program_cache = ProgramCache::new();
 0:19.73     |                         ^^^^^^^^^^^^^^^^^^^ expected 1 parameter
 0:19.73
 0:19.92 error: aborting due to previous error
 0:19.92

Comment 34

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Serialize ProgramBinary to/from blob/disk

Rebased.

Comment 35

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Serialize ProgramBinary to/from blob/disk

Fixed build problem.

Comment 36

[Sotaro Ikeda [:sotaro]]

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

Comment 37

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8981045 [details] [diff] [review]
patch - Serialize ProgramBinary to/from blob/disk

:nical, can you review the patch? :jrmuizel is on parental leave.

Comment 38

[Nicolas Silva [:nical]]

Comment on attachment 8981045 [details] [diff] [review]
patch - Serialize ProgramBinary to/from blob/disk

Review of attachment 8981045 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good overall, r=me with the {:?} debug formatters removed from release builds.

::: gfx/webrender_bindings/src/program_cache.rs
@@ +162,5 @@
> +            });
> +        }
> +    }
> +
> +    pub fn try_load_from_disk(&mut self, program_cache: &Rc<ProgramCache>) {

(Side note) as a next step it would be great to move this to the scene builder thread. I am a bit worried about doing any kind of disk io on the critical path.

@@ +177,5 @@
> +                for entry in read_dir(cache_path).unwrap() {
> +                    let entry = entry.unwrap();
> +                    let path = entry.path();
> +
> +                    info!("loading shader file {:?}", path.file_name());

There are concerns about using debug formatters in release build s they tend to add a lot of binary size bloats, see: https://groups.google.com/d/msg/mozilla.dev.servo/_gllnLmN_VU/CXh5EIy1AAAJ
Could you change this to be in debug builds only (with a generic message for release builds that provides less info, maybe)?

Same thing in various places in this patch.

Comment 39

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #38)
> ::: gfx/webrender_bindings/src/program_cache.rs
> @@ +162,5 @@
> > +            });
> > +        }
> > +    }
> > +
> > +    pub fn try_load_from_disk(&mut self, program_cache: &Rc<ProgramCache>) {
> 
> (Side note) as a next step it would be great to move this to the scene
> builder thread. I am a bit worried about doing any kind of disk io on the
> critical path.
> 

Thanks for the advice. Created Bug 1464958.

Comment 40

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #38)
> 
> @@ +177,5 @@
> > +                for entry in read_dir(cache_path).unwrap() {
> > +                    let entry = entry.unwrap();
> > +                    let path = entry.path();
> > +
> > +                    info!("loading shader file {:?}", path.file_name());
> 
> There are concerns about using debug formatters in release build s they tend
> to add a lot of binary size bloats, see:
> https://groups.google.com/d/msg/mozilla.dev.servo/_gllnLmN_VU/CXh5EIy1AAAJ
> Could you change this to be in debug builds only (with a generic message for
> release builds that provides less info, maybe)?
> 
> Same thing in various places in this patch.

I am going to remove {:?}.

Comment 41

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Serialize ProgramBinary to/from blob/disk

Comment 42

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Serialize ProgramBinary to/from blob/disk

Rebased.

Comment 43

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=83ac3a5e8fb0ac10e70d2e469a98125a38abcb64

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a34eb854c1c
Serialize ProgramBinary to/from blob/disk r=nical

Comment 45

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/1a34eb854c1c

Comment 46

[Ionuț Goldan [:igoldan]]

Looks like this bug brought some big perf improvements:

== Change summary for alert #13581 (as of Thu, 31 May 2018 05:51:08 GMT) ==

Improvements:

 55%  sessionrestore windows10-64-qr opt e10s stylo     1,017.04 -> 459.25
 17%  sessionrestore_many_windows windows10-64-qr opt e10s stylo3,484.17 -> 2,896.25

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=13581

Comment 47

[Aria Beingessner [:Gankra]]

yep, those were expected, awesome!