Closed Bug 1418202 Opened 6 years ago Closed 6 years ago

Serialize ProgramBinary to/from blob/disk

Categories

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

Product:
Core
Component:
Graphics: WebRender
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
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)

temporal patch - Add log to measure shader creation time
1.64 KB, patch
Details | Diff | Splinter Review
diagram of current implementation
95.99 KB, text/plain
Details
patch - Serialize ProgramBinary to/from blob/disk
29.78 KB, patch
sotaro
: review+
Details | Diff | Splinter Review 
This bug handles ProgramBinary to/from blob/disk.
Depends on: 1391159
Blocks: stage-wr-nightly
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: 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)
(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]
Blocks: 1411503
I've filed https://github.com/servo/webrender/issues/2580 about doing shader processing at build time.
From https://github.com/servo/webrender/issues/2580, it seems better to implement saving ProgramBinary to disc.
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+
(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.
(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.
The diagram explains things well and the design looks good.
Thanks for checking!
Update a bit.
Attachment #8975772 - Attachment is obsolete: true
Attachment #8976401 - Attachment is patch: false
Attached patch wip patch (obsolete) — — Splinter Review 

        Attachment #8976422 -
        Attachment is obsolete: true

    
  
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

    
    

    
  


  
Rebased.

        Attachment #8979156 -
        Attachment is obsolete: true

    
    

    
  


  
Fixed build problem.

        Attachment #8981035 -
        Attachment is obsolete: true

    
    

    
  
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+

    
    

    
  
(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.

    
  
Blocks: 1464958

    
    

    
  
(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 {:?}.

    
    

    
      
  


  

    
    

    
  


  
Rebased.

        Attachment #8981301 -
        Attachment is obsolete: true

        Attachment #8981305 -
        Flags: review+

    
    

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

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/1a34eb854c1c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago

          status-firefox62:
          --- → fixed
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!

          You need to log in
          before you can comment on or make changes to this bug.