Improve MediaCacheStream::NotifyDataReceived() by saving unnecessary copies

RESOLVED FIXED in Firefox 55

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
http://searchfox.org/mozilla-central/rev/624d25b2980cf5f83452b040e6d664460f9b7ec5/dom/media/MediaCache.cpp#1821

By making FileBlockCache::WriteBlock() take 2 input buffers instead of one, we are able to write bytes in |PartialBlockBuffer| and |data| to the cache without copying |data| to |PartialBlockBuffer| first.

This allows us to save one copy in this case.
(Assignee)

Updated

2 years ago
Assignee: nobody → jwwang
Priority: -- → P3
(Assignee)

Updated

2 years ago
Blocks: 1355409
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8857296 - Flags: review?(cpearce)
Attachment #8857297 - Flags: review?(cpearce)

Comment 3

2 years ago
mozreview-review
Comment on attachment 8857296 [details]
Bug 1355393. P1 - let FileBlockCache::WriteBlock() take 2 buffers instead of one.

https://reviewboard.mozilla.org/r/129256/#review132290

::: dom/media/MediaCache.cpp:1818
(Diff revision 1)
>      }
>  
>      if (blockDataToStore) {
> -      gMediaCache->AllocateAndWriteBlock(this, blockDataToStore, mode);
> +      const uint8_t* p = reinterpret_cast<const uint8_t*>(blockDataToStore);
> +      auto data = MakeSpan<const uint8_t>(p, BLOCK_SIZE);
> +      gMediaCache->AllocateAndWriteBlock(this, mode, data, data.First(0));

Might be cleaner to have a polymorphic overloads of AllcoateAndWriteBlock() and WriteBlock() that takes only 1 span, and which pass MakeSpan<const uint8_t>(nullptr, 0) as the second argument to the two argument variant.
Attachment #8857296 - Flags: review?(cpearce) → review+
(Assignee)

Comment 4

2 years ago
mozreview-review-reply
Comment on attachment 8857296 [details]
Bug 1355393. P1 - let FileBlockCache::WriteBlock() take 2 buffers instead of one.

https://reviewboard.mozilla.org/r/129256/#review132290

> Might be cleaner to have a polymorphic overloads of AllcoateAndWriteBlock() and WriteBlock() that takes only 1 span, and which pass MakeSpan<const uint8_t>(nullptr, 0) as the second argument to the two argument variant.

Default arguments should do the job. I will try to figure it out.

Comment 5

2 years ago
mozreview-review
Comment on attachment 8857297 [details]
Bug 1355393. P2 - write the bytes out when we've collected a whole block.

https://reviewboard.mozilla.org/r/129258/#review132300
Attachment #8857297 - Flags: review?(cpearce) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

2 years ago
Thanks for the review!

Comment 9

2 years ago
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/98b2e2843d5b
P1 - let FileBlockCache::WriteBlock() take 2 buffers instead of one. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/feb790f0853c
P2 - write the bytes out when we've collected a whole block. r=cpearce

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/98b2e2843d5b
https://hg.mozilla.org/mozilla-central/rev/feb790f0853c
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.