1304303 - Speed up default initialization of SampleInfoSize table.

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P3)

Description

[Matthew Gregan [:kinetik]]

I noticed the stagefright_* GTests taking a long time to run in my local debug builds.  Concerned that it might be the Rust parser, I did some quick and dirty gdb ctrl-c profiling and found most of the time wasted growing Saiz::mSampleInfoSize in small increments.  Since we know the final size, it makes sense to simply allocate the table up-front and then initialize the elements with the default value.

This reduces the debug build runtime of 'mach gtest stagefright_*' from >1m30s to <16s locally.

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: patch v0

MozReview-Commit-ID: 9dValMxqVkh

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8793218 [details] [diff] [review]
patch v0

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

Didn't know that ReplaceElementsAt with one value, thank you.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b7d9479690e
Speed up default initialization of SampleInfoSize table.  r=gerald

Comment 4

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a03ca4ae130b for a simple straightforward gtest https://treeherder.mozilla.org/logviewer.html#?job_id=36261460&repo=mozilla-inbound and a variety of mda, https://treeherder.mozilla.org/logviewer.html#?job_id=36263807&repo=mozilla-inbound or https://treeherder.mozilla.org/logviewer.html#?job_id=36260570&repo=mozilla-inbound or some I've already starred without realizing they were new.

Comment 5

[Matthew Gregan [:kinetik]]

(In reply to Gerald Squelart [:gerald] from comment #2)
> Didn't know that ReplaceElementsAt with one value, thank you.

Turns out not to do what I wanted -- I thought it was equivalent to std::vector::assign.  Instead, it replaces the specified range with one supplied value, i.e. the specified range is removed and the single supplied value is inserted.

Comment 6

[Matthew Gregan [:kinetik]]

Attachment: patch v1

Comment 7

[Matthew Gregan [:kinetik]]

I can't find a clean way to do this without doing the work of extending nsTArray's interface to add something like std::vector::assign.

For quick fixes, it's a question of how much time to save running debug build tests on infrastructure vs code cleanliness, since I doubt this change will result in much real-world performance improvement.

Replacing ReplaceElementsAt with:

     for (auto i = 0; i < mSampleInfoSize.Length(); ++i) {
       mSampleInfoSize[i] = defaultSampleInfoSize;
     }

...drops the gtest from ~1:30s to ~46s.  Most of the remaining time is spent in nsTArray bounds checking.

Replacing that with:

     uint8_t* p = mSampleInfoSize.Elements();
     for (auto i = 0; i < count; ++i) {
       p[i] = defaultSampleInfoSize;
     }

...drops the gtest down to ~23s.  This is all debug builds, obviously there's going to be much less difference with optimized builds.

Comment 8

[Matthew Gregan [:kinetik]]

patch v1 takes the first option, but if you think it's worth taking the second option, let me know.

Comment 9

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8793613 [details] [diff] [review]
patch v1

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

Happy with either (as you say, the non-debug version should end up being equal), so r+.

Maybe a 3rd option, if you have time to benchmark it: Starting like your 2nd (get a pointer on the elements once), but then since we know the elements are bytes, you could use std::memset, which can benefit from platform-dependent optimizations (e.g. write correctly-aligned processor words).

Comment 10

[Matthew Gregan [:kinetik]]

(In reply to Gerald Squelart [:gerald] from comment #9)

> Happy with either (as you say, the non-debug version should end up being
> equal), so r+.
> 
> Maybe a 3rd option, if you have time to benchmark it: Starting like your 2nd
> (get a pointer on the elements once), but then since we know the elements
> are bytes, you could use std::memset, which can benefit from
> platform-dependent optimizations (e.g. write correctly-aligned processor
> words).

Ah yes, of course.  Thanks!  That's at ~21s, and probably the cleanest, so I'll assume r+ and push that variant.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/036c39d5be0a
Speed up default initialization of SampleInfoSize table.  r=gerald

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/036c39d5be0a