Closed Bug 1391159 Opened 2 years ago Closed 2 years ago

Shader creation of WebRender took long time

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox56 --- unaffected
firefox57 --- unaffected
firefox59 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

(Keywords: stale-bug, Whiteboard: [wr-mvp])

Attachments

(3 files, 14 obsolete files)

2.90 KB, text/plain
Details
11.01 KB, patch
nical
: review+
Details | Diff | Splinter Review
1.63 KB, patch
Details | Diff | Splinter Review
When WebRender is enabled, some shader creations happened on 2nd rendering. Each shader creation took 50ms - 150ms, then 2nd webrender rendering took about 600ms - 1000ms. The duration of 2nd composition was depended on PC.
I used the patch to quickly measure shaders creation time.
Assignee: nobody → sotaro.ikeda.g
Assignee: sotaro.ikeda.g → nobody
Measured shaders creation time with attachment 8898127 [details] [diff] [review]. It was measured on Lenovo P50(windows).

Each shader creation took 43 ms - 151 ms and 2nd webrender rendering took 841ms.
The following is an advice by Jeff Gilbert via E-mail.

-------------------------------------------------------------------------

My instinct here is that WR should keep a global shader cache itself.
We do want a shader cache system for WebGL as well, so if we can kill
two birds with one stone here, that would be a plus.

I don't think we should try to embed shader caching in GLContext transparently.

Straw-man:

class ShaderProgramCache
{
  map<Hasher::HashT, RefPtr<ProgramBinary>> cache;

  struct ProgramBinary : public RefCounted<ProgramBinary> {
    GLenum format;
    UniqueBuffer bytes;
    uint32_t byteLen;
  };

  void Link(GLContext* gl, GLuint progId, const char* vertSource,
const char* fragSource) {
    Hasher hasher;
    hasher.digest(gl->mRendererString);
    hasher.digest(vertSource);
    hasher.digest(fragSource);
    const auto hash = hasher.get();
    const auto itr = cache.find(hash);
    if (itr != cache.end()) {
      const auto& bin = itr->second;
      gl->fProgramBinary(progId, bin->format, bin->bytes, bin->byteLen);
      return;
    }
    gl->fLinkProgram(progId);
    GLenum status = 0;
    gl->fGetProgramiv(progId, LOCAL_GL_LINK_STATUS, (GLint*)&status);
    if (!status)
      return;
    RefPtr<ProgramBinary> bin = new ProgramBinary;
    gl->fGetProgramiv(progId, LOCAL_GL_PROGRAM_BINARY_LENGTH,
(GLint*)&bin->byteLen);
    bin->bytes = malloc(bin->byteLen);
    if (!bin->bytes)
      return;
    gl->fGetProgramBinary(progId, bin->byteLen, nullptr, &bin->format,
bin->bytes.get());
   cache.insert({hash, bin});
  }
};

Optional requirements:
- Async linking
- Serialize to/from blob/disk
Attachment #8898130 - Attachment description: log - mearured shader creation time → log - mearured shader creation time on Lenovo P50
I am going to take a look.
Assignee: nobody → sotaro.ikeda.g
See Also: → 1370885
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [wr-mvp]
Target Milestone: --- → mozilla57
Rebased.
Attachment #8898127 - Attachment is obsolete: true
Attached patch wip (obsolete) — Splinter Review
Without glProgramBinary usage, each shader compiling took 60ms - 500ms with ANGLE on P90(Win10).

With glProgramBinary usage, each shader load became 4ms - 10ms with ANGLE on P90(Win10).
Attached patch wip (obsolete) — Splinter Review
Attachment #8925862 - Attachment is obsolete: true
(In reply to Sotaro Ikeda [:sotaro] from comment #3)
> The following is an advice by Jeff Gilbert via E-mail.
> 
> -------------------------------------------------------------------------
> 
> I don't think we should try to embed shader caching in GLContext
> transparently.
> 
> Optional requirements:
> - Async linking
> - Serialize to/from blob/disk

:jgilbert, can you explain more about "Async linking"? Thanks!
Flags: needinfo?(jgilbert)
Attached patch wip (obsolete) — Splinter Review
Attachment #8926184 - Attachment is obsolete: true
Attachment #8927700 - Attachment description: patch_tmp_binary_10 → wip
Attached patch wip (obsolete) — Splinter Review
Attachment #8927700 - Attachment is obsolete: true
Attached patch wip (obsolete) — Splinter Review
Attachment #8927747 - Attachment is obsolete: true
(In reply to Sotaro Ikeda [:sotaro] from comment #3)
> 
> Optional requirements:
> - Async linking
> - Serialize to/from blob/disk

Optional requirements is going to be handled in different bugs.
(In reply to Sotaro Ikeda [:sotaro] from comment #14)
> (In reply to Sotaro Ikeda [:sotaro] from comment #3)
> > 
> > - Serialize to/from blob/disk

Bug 1418202 is created for it.
Blocks: 1418202
Attached patch wip (obsolete) — Splinter Review
Attachment #8928483 - Attachment is obsolete: true
Attachment #8929358 - Attachment is obsolete: true
For now, ProgramBinary is necessary for windows. On my linux PC, I did not see performance improvement.
Comment on attachment 8929359 [details] [diff] [review]
patch - handle WebRender ProgramBinary usage

:nical, can you feedback to the patch?
Attachment #8929359 - Flags: feedback?(nical.bugzilla)
Comment on attachment 8929359 [details] [diff] [review]
patch - handle WebRender ProgramBinary usage

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

Looks sane.
Attachment #8929359 - Flags: feedback?(nical.bugzilla) → feedback+
Depends on: 1418315
Attached patch wip (obsolete) — Splinter Review
Attachment #8929353 - Attachment is obsolete: true
No longer depends on: 1418315
Attachment #8929359 - Attachment is obsolete: true
Attached patch wip (obsolete) — Splinter Review
Attachment #8930062 - Attachment is obsolete: true
Depends on: 1419440
Attachment #8925813 - Attachment description: patch - Add log to measure shader creation time → temporal patch - Add log to measure shader creation time
Attachment #8930367 - Attachment is obsolete: true
Attachment #8930369 - Attachment is obsolete: true
Attachment #8931569 - Attachment is obsolete: true
Attachment #8931570 - Attachment description: patch - handle WebRender ProgramBinary usage → patch - Handle WebRender ProgramBinary usage
Attachment #8931570 - Flags: review?(nical.bugzilla)
Comment on attachment 8931570 [details] [diff] [review]
patch - Handle WebRender ProgramBinary usage

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

Looks good!
Attachment #8931570 - Flags: review?(nical.bugzilla) → review+
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e60ad275b73
Handle WebRender ProgramBinary usage r=nical
https://hg.mozilla.org/mozilla-central/rev/7e60ad275b73
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
(In reply to Sotaro Ikeda [:sotaro] from comment #10)
> (In reply to Sotaro Ikeda [:sotaro] from comment #3)
> > The following is an advice by Jeff Gilbert via E-mail.
> > 
> > -------------------------------------------------------------------------
> > 
> > I don't think we should try to embed shader caching in GLContext
> > transparently.
> > 
> > Optional requirements:
> > - Async linking
> > - Serialize to/from blob/disk
> 
> :jgilbert, can you explain more about "Async linking"? Thanks!

Async linking is where we call LinkProgram for all programs before querying the LINK_STATUS for any of them. This lets the driver do linking in parallel. I believe ANGLE supports this, or something similar.
Flags: needinfo?(jgilbert)
You need to log in before you can comment on or make changes to this bug.