Serialize ProgramBinary to/from blob/disk

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: sotaro, Assigned: sotaro)

Tracking

(Blocks 1 bug)

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 unaffected, firefox62 fixed)

Details

(Whiteboard: [wr-reserve] [gfx-noted])

Attachments

(3 attachments, 23 obsolete attachments)

1.64 KB, patch
Details | Diff | Splinter Review
95.99 KB, text/plain
Details
29.78 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
Assignee

Description

2 years ago
This bug handles ProgramBinary to/from blob/disk.
Assignee

Updated

2 years ago
Depends on: 1391159
Assignee

Updated

2 years ago
Whiteboard: [wr-mvp] [triage]
Summary: Serialize serialize ProgramBinary to/from blob/disk → Serialize ProgramBinary to/from blob/disk
Whiteboard: [wr-mvp] [triage] → [wr-mvp]
Whiteboard: [wr-mvp] → [wr-mvp] [gfx-noted]
Assignee

Updated

2 years ago
Assignee: nobody → sotaro.ikeda.g
Status: NEW → ASSIGNED
Priority: P2 → P1
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.
Flags: needinfo?(sotaro.ikeda.g)
Assignee

Comment 2

2 years ago
(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.
Flags: needinfo?(sotaro.ikeda.g)
Whiteboard: [wr-mvp] [gfx-noted] → [wr-reserve] [gfx-noted]
Assignee

Updated

a year ago
Blocks: 1411503
I've filed https://github.com/servo/webrender/issues/2580 about doing shader processing at build time.
Assignee

Comment 4

a year ago
From https://github.com/servo/webrender/issues/2580, it seems better to implement saving ProgramBinary to disc.
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Assignee

Comment 19

a year ago
Comment on attachment 8971527 [details] [diff] [review]
wip patch

:jrmuizel, can you feedback to the patch? Though it is still wip.
Attachment #8971527 - Flags: feedback?(jmuizelaar)
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 #8971527 - Flags: feedback?(jmuizelaar) → feedback+
Assignee

Comment 21

a year ago
(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 hidden (spam)
Comment hidden (obsolete)
Comment hidden (spam)
Comment hidden (obsolete)
Assignee

Comment 26

a year ago
(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 hidden (obsolete)
The diagram explains things well and the design looks good.
Assignee

Comment 29

a year ago
Thanks for checking!
Assignee

Comment 30

a year ago
Update a bit.
Attachment #8975772 - Attachment is obsolete: true
Assignee

Updated

a year ago
Attachment #8976401 - Attachment is patch: false
Comment hidden (obsolete)
Assignee

Comment 32

a year ago
Posted patch wip patch (obsolete) — Splinter Review
Attachment #8976422 - Attachment is obsolete: true
Assignee

Updated

a year ago
Depends on: 1463416
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
No longer depends on: 1463416
Assignee

Comment 34

a year ago
Rebased.
Attachment #8979156 - Attachment is obsolete: true
Assignee

Comment 35

a year ago
Fixed build problem.
Attachment #8981035 - Attachment is obsolete: true
Assignee

Comment 37

a year ago
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.
Attachment #8981045 - Flags: review?(nical.bugzilla)
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.
Attachment #8981045 - Flags: review?(nical.bugzilla) → review+
Assignee

Comment 39

a year ago
(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.
Assignee

Updated

a year ago
Blocks: 1464958
Assignee

Comment 40

a year ago
(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 hidden (obsolete)
Assignee

Comment 42

a year ago
Rebased.
Attachment #8981301 - Attachment is obsolete: true
Attachment #8981305 - Flags: review+

Comment 44

a year ago
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a34eb854c1c
Serialize ProgramBinary to/from blob/disk r=nical

Comment 45

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1a34eb854c1c
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
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
yep, those were expected, awesome!
Duplicate of this bug: 1465595
You need to log in before you can comment on or make changes to this bug.