Closed
Bug 1418202
Opened 6 years ago
Closed 5 years ago
Serialize ProgramBinary to/from blob/disk
Categories
(Core :: Graphics: WebRender, enhancement, P2)
Core
Graphics: WebRender
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox57 | --- | unaffected |
firefox62 | --- | fixed |
People
(Reporter: sotaro, Assigned: sotaro)
References
(Blocks 1 open bug)
Details
(Whiteboard: [wr-reserve] [gfx-noted])
Attachments
(3 files, 23 obsolete files)
1.64 KB,
patch
|
Details | Diff | Splinter Review | |
95.99 KB,
text/plain
|
Details | |
29.78 KB,
patch
|
sotaro
:
review+
|
Details | Diff | Splinter Review |
This bug handles ProgramBinary to/from blob/disk.
Assignee | ||
Updated•6 years ago
|
Blocks: stage-wr-nightly
Updated•6 years ago
|
Whiteboard: [wr-mvp] [triage]
Updated•6 years ago
|
status-firefox57:
--- → unaffected
Priority: -- → P2
Updated•6 years ago
|
Summary: Serialize serialize ProgramBinary to/from blob/disk → Serialize ProgramBinary to/from blob/disk
Updated•6 years ago
|
Whiteboard: [wr-mvp] [triage] → [wr-mvp]
Updated•6 years ago
|
Whiteboard: [wr-mvp] → [wr-mvp] [gfx-noted]
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → sotaro.ikeda.g
Updated•6 years ago
|
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment 1•6 years ago
|
||
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•6 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)
Updated•6 years ago
|
Whiteboard: [wr-mvp] [gfx-noted] → [wr-reserve] [gfx-noted]
Updated•6 years ago
|
Priority: P1 → P3
Updated•6 years ago
|
Priority: P3 → P2
Comment 3•5 years ago
|
||
I've filed https://github.com/servo/webrender/issues/2580 about doing shader processing at build time.
Assignee | ||
Updated•5 years ago
|
See Also: → https://github.com/servo/webrender/issues/2580
Assignee | ||
Comment 4•5 years ago
|
||
From https://github.com/servo/webrender/issues/2580, it seems better to implement saving ProgramBinary to disc.
Comment hidden (spam) |
Comment hidden (spam) |
Assignee | ||
Comment 7•5 years ago
|
||
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•5 years 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 20•5 years ago
|
||
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•5 years 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•5 years 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) |
Comment 28•5 years ago
|
||
The diagram explains things well and the design looks good.
Assignee | ||
Comment 29•5 years ago
|
||
Thanks for checking!
Assignee | ||
Updated•5 years ago
|
Attachment #8976401 -
Attachment is patch: false
Comment hidden (obsolete) |
Assignee | ||
Updated•5 years ago
|
See Also: → https://github.com/servo/webrender/pull/2764
Assignee | ||
Comment 32•5 years ago
|
||
Attachment #8976422 -
Attachment is obsolete: true
Comment 33•5 years ago
|
||
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
Assignee | ||
Comment 35•5 years ago
|
||
Fixed build problem.
Attachment #8981035 -
Attachment is obsolete: true
Assignee | ||
Comment 36•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6084854af53ad47cb4ec7ba83c4b2bc881573e8
Assignee | ||
Comment 37•5 years 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 38•5 years ago
|
||
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•5 years 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 | ||
Comment 40•5 years 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•5 years ago
|
||
Rebased.
Attachment #8981301 -
Attachment is obsolete: true
Attachment #8981305 -
Flags: review+
Assignee | ||
Comment 43•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=83ac3a5e8fb0ac10e70d2e469a98125a38abcb64
Comment 44•5 years 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•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1a34eb854c1c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Comment 46•5 years ago
|
||
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•5 years ago
|
||
yep, those were expected, awesome!
You need to log in
before you can comment on or make changes to this bug.
Description
•