Cache the subtype of each DecoderInfo to make the Is* checks quicker.

Cache the subtype of each DecoderInfo to make the Is* checks quicker.

Addresses a regression in the NetEq performance test.

# Added NOTRY due to android_arm64_rel being swamped.
NOTRY=True
BUG=chromium:651426
Committed: https://crrev.com/9f38c218ee0282042e5bc31d3d3a1b1f53e4be16
Cr-Commit-Position: refs/heads/master@{#14495}

[ossu, 2016-09-30 11:02:22]

ossu@webrtc.org changed reviewers:
+ kwiberg@webrtc.org

[ossu, 2016-09-30 11:02:23]

Putting this up mainly for discussion. The recent change to DecoderDatabase
caused a regression of about 5% on the neteq performance test. I believe it's
linked to the performance of the Is* methods on DecoderInfo. They're called
several times per packet by NetEq, so any slight changes to them are likely to
show up in the test, which essentially just pushes raw audio through the NetEq
pipeline. It's unclear to me how large an impact this has in a real scenario
(i.e. where encoding and playout is involved).

I've improved these methods by precalculating their returns on object
initialization. My question is: is this change worth it, or should we delay it
until the dust has settled on the remodeling we're currently doing of the audio
chain?

[kwiberg-webrtc, 2016-09-30 11:27:04]

On 2016/09/30 11:02:23, ossu wrote:
> Putting this up mainly for discussion. The recent change to DecoderDatabase
> caused a regression of about 5% on the neteq performance test. I believe it's
> linked to the performance of the Is* methods on DecoderInfo. They're called
> several times per packet by NetEq, so any slight changes to them are likely to
> show up in the test, which essentially just pushes raw audio through the NetEq
> pipeline.

Did you check what effect this CL has on that benchmark?

> It's unclear to me how large an impact this has in a real scenario
> (i.e. where encoding and playout is involved).
> 
> I've improved these methods by precalculating their returns on object
> initialization. My question is: is this change worth it, or should we delay it
> until the dust has settled on the remodeling we're currently doing of the
audio
> chain?

I don't think we're going to change anything that would render the changes in
this CL obsolete. So I think it just comes down to whether the caching actually
solves the regression.

[kwiberg-webrtc, 2016-09-30 11:28:24]

https://codereview.webrtc.org/2383723002/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/decoder_database.h (right):

https://codereview.webrtc.org/2383723002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/decoder_database.h:106: };
Do you have to tell the compiler to store this in an 8-bit number, or do you get
that by default? (Is the answer to that question different for enum and enum
class?)

[ossu, 2016-09-30 11:31:47]

On 2016/09/30 11:27:04, kwiberg-webrtc wrote:
> On 2016/09/30 11:02:23, ossu wrote:
> > Putting this up mainly for discussion. The recent change to DecoderDatabase
> > caused a regression of about 5% on the neteq performance test. I believe
it's
> > linked to the performance of the Is* methods on DecoderInfo. They're called
> > several times per packet by NetEq, so any slight changes to them are likely
to
> > show up in the test, which essentially just pushes raw audio through the
NetEq
> > pipeline.
> 
> Did you check what effect this CL has on that benchmark?
> 
> > It's unclear to me how large an impact this has in a real scenario
> > (i.e. where encoding and playout is involved).
> > 
> > I've improved these methods by precalculating their returns on object
> > initialization. My question is: is this change worth it, or should we delay
it
> > until the dust has settled on the remodeling we're currently doing of the
> audio
> > chain?
> 
> I don't think we're going to change anything that would render the changes in
> this CL obsolete. So I think it just comes down to whether the caching
actually
> solves the regression.

Ah, I see I failed to mention that: Yes. On my machine (at least) these changes
puts the performance results back to where they were before this change. It
would be silly of me to propose a fix if it weren't actually a fix. However, I'm
quite often a bit silly. :)

[ossu, 2016-09-30 11:36:48]

https://codereview.webrtc.org/2383723002/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/decoder_database.h (right):

https://codereview.webrtc.org/2383723002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/decoder_database.h:106: };
On 2016/09/30 11:28:24, kwiberg-webrtc wrote:
> Do you have to tell the compiler to store this in an 8-bit number, or do you
get
> that by default? (Is the answer to that question different for enum and enum
> class?)

According to a quick look-on-the-internet, enum class defaults to int. I can set
it to be 8 bits, but I'm not sure what would be gained from it. The size of
DecoderInfo (with padding) should remain the same and I'm not sure we'll be able
to access an int8_t any faster in general.

[kwiberg-webrtc, 2016-09-30 11:37:11]

On 2016/09/30 11:31:47, ossu wrote:
> On 2016/09/30 11:27:04, kwiberg-webrtc wrote:
> > On 2016/09/30 11:02:23, ossu wrote:
> > > Putting this up mainly for discussion. The recent change to
DecoderDatabase
> > > caused a regression of about 5% on the neteq performance test. I believe
> it's
> > > linked to the performance of the Is* methods on DecoderInfo. They're
called
> > > several times per packet by NetEq, so any slight changes to them are
likely
> to
> > > show up in the test, which essentially just pushes raw audio through the
> NetEq
> > > pipeline.
> > 
> > Did you check what effect this CL has on that benchmark?
> > 
> > > It's unclear to me how large an impact this has in a real scenario
> > > (i.e. where encoding and playout is involved).
> > > 
> > > I've improved these methods by precalculating their returns on object
> > > initialization. My question is: is this change worth it, or should we
delay
> it
> > > until the dust has settled on the remodeling we're currently doing of the
> > audio
> > > chain?
> > 
> > I don't think we're going to change anything that would render the changes
in
> > this CL obsolete. So I think it just comes down to whether the caching
> actually
> > solves the regression.
> 
> Ah, I see I failed to mention that: Yes. On my machine (at least) these
changes
> puts the performance results back to where they were before this change. It
> would be silly of me to propose a fix if it weren't actually a fix. However,
I'm
> quite often a bit silly. :)

So then it's just not a belief that the regression is linked to the performance
of the Is* methods on DecoderInfo... :-)

Well then. Ship it! lgtm.

[ossu, 2016-09-30 11:40:38]

The CQ bit was checked by ossu@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 11:40:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[kwiberg-webrtc, 2016-09-30 11:45:37]

https://codereview.webrtc.org/2383723002/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/decoder_database.h (right):

https://codereview.webrtc.org/2383723002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/decoder_database.h:106: };
On 2016/09/30 11:36:47, ossu wrote:
> On 2016/09/30 11:28:24, kwiberg-webrtc wrote:
> > Do you have to tell the compiler to store this in an 8-bit number, or do you
> get
> > that by default? (Is the answer to that question different for enum and enum
> > class?)
> 
> According to a quick look-on-the-internet, enum class defaults to int. I can
set
> it to be 8 bits, but I'm not sure what would be gained from it. The size of
> DecoderInfo (with padding) should remain the same and I'm not sure we'll be
able
> to access an int8_t any faster in general.

Look at it this way: reading and writing 1 byte is at least as cheap as reading
or writing 4 bytes on hardware that we care about, and unless you start to
really track these things you never know when it'll be an actual space saving.

Your call.

[ossu, 2016-09-30 11:50:34]

https://codereview.webrtc.org/2383723002/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/decoder_database.h (right):

https://codereview.webrtc.org/2383723002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/decoder_database.h:106: };
On 2016/09/30 11:45:37, kwiberg-webrtc wrote:
> On 2016/09/30 11:36:47, ossu wrote:
> > On 2016/09/30 11:28:24, kwiberg-webrtc wrote:
> > > Do you have to tell the compiler to store this in an 8-bit number, or do
you
> > get
> > > that by default? (Is the answer to that question different for enum and
enum
> > > class?)
> > 
> > According to a quick look-on-the-internet, enum class defaults to int. I can
> set
> > it to be 8 bits, but I'm not sure what would be gained from it. The size of
> > DecoderInfo (with padding) should remain the same and I'm not sure we'll be
> able
> > to access an int8_t any faster in general.
> 
> Look at it this way: reading and writing 1 byte is at least as cheap as
reading
> or writing 4 bytes on hardware that we care about, and unless you start to
> really track these things you never know when it'll be an actual space saving.
> 
> Your call.

Just the other day I was asked to change two uint8_t in Priority to int and now
we want it the other way around? The world is strange and confusing! :(

[kwiberg-webrtc, 2016-09-30 13:05:27]

https://codereview.webrtc.org/2383723002/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/decoder_database.h (right):

https://codereview.webrtc.org/2383723002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/decoder_database.h:106: };
On 2016/09/30 11:50:34, ossu wrote:
> On 2016/09/30 11:45:37, kwiberg-webrtc wrote:
> > On 2016/09/30 11:36:47, ossu wrote:
> > > On 2016/09/30 11:28:24, kwiberg-webrtc wrote:
> > > > Do you have to tell the compiler to store this in an 8-bit number, or do
> you
> > > get
> > > > that by default? (Is the answer to that question different for enum and
> enum
> > > > class?)
> > > 
> > > According to a quick look-on-the-internet, enum class defaults to int. I
can
> > set
> > > it to be 8 bits, but I'm not sure what would be gained from it. The size
of
> > > DecoderInfo (with padding) should remain the same and I'm not sure we'll
be
> > able
> > > to access an int8_t any faster in general.
> > 
> > Look at it this way: reading and writing 1 byte is at least as cheap as
> reading
> > or writing 4 bytes on hardware that we care about, and unless you start to
> > really track these things you never know when it'll be an actual space
saving.
> > 
> > Your call.
> 
> Just the other day I was asked to change two uint8_t in Priority to int and
now
> we want it the other way around? The world is strange and confusing! :(

The reason we prefer int for integers rather than something smaller (or
unsigned) is that less confusing things happen when the range in practice turns
out to be larger than it's supposed to be (e.g. because there's a bug).

For enums, especially enum class, you never do any arithmetic or things like
that, so the problems that we're protecting against by picking a larger type
can't happen.

Or at least that's my interpretation.

[commit-bot: I haz the power, 2016-09-30 13:41:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 13:41:18]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[ossu, 2016-09-30 13:45:22]

https://codereview.webrtc.org/2383723002/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/decoder_database.h (right):

https://codereview.webrtc.org/2383723002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/decoder_database.h:106: };
On 2016/09/30 13:05:27, kwiberg-webrtc wrote:
> On 2016/09/30 11:50:34, ossu wrote:
> > On 2016/09/30 11:45:37, kwiberg-webrtc wrote:
> > > On 2016/09/30 11:36:47, ossu wrote:
> > > > On 2016/09/30 11:28:24, kwiberg-webrtc wrote:
> > > > > Do you have to tell the compiler to store this in an 8-bit number, or
do
> > you
> > > > get
> > > > > that by default? (Is the answer to that question different for enum
and
> > enum
> > > > > class?)
> > > > 
> > > > According to a quick look-on-the-internet, enum class defaults to int. I
> can
> > > set
> > > > it to be 8 bits, but I'm not sure what would be gained from it. The size
> of
> > > > DecoderInfo (with padding) should remain the same and I'm not sure we'll
> be
> > > able
> > > > to access an int8_t any faster in general.
> > > 
> > > Look at it this way: reading and writing 1 byte is at least as cheap as
> > reading
> > > or writing 4 bytes on hardware that we care about, and unless you start to
> > > really track these things you never know when it'll be an actual space
> saving.
> > > 
> > > Your call.
> > 
> > Just the other day I was asked to change two uint8_t in Priority to int and
> now
> > we want it the other way around? The world is strange and confusing! :(
> 
> The reason we prefer int for integers rather than something smaller (or
> unsigned) is that less confusing things happen when the range in practice
turns
> out to be larger than it's supposed to be (e.g. because there's a bug).
> 
> For enums, especially enum class, you never do any arithmetic or things like
> that, so the problems that we're protecting against by picking a larger type
> can't happen.
> 
> Or at least that's my interpretation.

Alright. I've turned it into an int8_t and a quick comparison run shows a small
(likely statistically insignificant) improvement on my workstation.

[ossu, 2016-09-30 13:45:29]

The CQ bit was checked by ossu@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 13:45:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-30 15:46:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 15:46:11]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[kwiberg-webrtc, 2016-09-30 16:55:00]

https://codereview.webrtc.org/2383723002/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/decoder_database.h (right):

https://codereview.webrtc.org/2383723002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/decoder_database.h:106: };
On 2016/09/30 13:45:21, ossu wrote:
> On 2016/09/30 13:05:27, kwiberg-webrtc wrote:
> > On 2016/09/30 11:50:34, ossu wrote:
> > > On 2016/09/30 11:45:37, kwiberg-webrtc wrote:
> > > > On 2016/09/30 11:36:47, ossu wrote:
> > > > > On 2016/09/30 11:28:24, kwiberg-webrtc wrote:
> > > > > > Do you have to tell the compiler to store this in an 8-bit number,
or
> do
> > > you
> > > > > get
> > > > > > that by default? (Is the answer to that question different for enum
> and
> > > enum
> > > > > > class?)
> > > > > 
> > > > > According to a quick look-on-the-internet, enum class defaults to int.
I
> > can
> > > > set
> > > > > it to be 8 bits, but I'm not sure what would be gained from it. The
size
> > of
> > > > > DecoderInfo (with padding) should remain the same and I'm not sure
we'll
> > be
> > > > able
> > > > > to access an int8_t any faster in general.
> > > > 
> > > > Look at it this way: reading and writing 1 byte is at least as cheap as
> > > reading
> > > > or writing 4 bytes on hardware that we care about, and unless you start
to
> > > > really track these things you never know when it'll be an actual space
> > saving.
> > > > 
> > > > Your call.
> > > 
> > > Just the other day I was asked to change two uint8_t in Priority to int
and
> > now
> > > we want it the other way around? The world is strange and confusing! :(
> > 
> > The reason we prefer int for integers rather than something smaller (or
> > unsigned) is that less confusing things happen when the range in practice
> turns
> > out to be larger than it's supposed to be (e.g. because there's a bug).
> > 
> > For enums, especially enum class, you never do any arithmetic or things like
> > that, so the problems that we're protecting against by picking a larger type
> > can't happen.
> > 
> > Or at least that's my interpretation.
> 
> Alright. I've turned it into an int8_t and a quick comparison run shows a
small
> (likely statistically insignificant) improvement on my workstation.

Wohoo! That's the best kind of improvement, right?

[ossu, 2016-10-03 08:33:56]

The CQ bit was checked by ossu@webrtc.org

[ossu, 2016-10-03 08:33:56]

The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2383723002/#ps20001 (title:
"Make the DecoderInfo::Subtype enum 8 bits in size.")

[commit-bot: I haz the power, 2016-10-03 08:34:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-10-03 10:34:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-03 10:34:25]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[ossu, 2016-10-03 12:51:31]

The CQ bit was checked by ossu@webrtc.org

[ossu, 2016-10-03 12:51:32]

The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2383723002/#ps40001 (title:
"Since we're already shaving the pig...")

[commit-bot: I haz the power, 2016-10-03 12:51:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-10-03 14:52:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-03 14:52:13]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[ossu, 2016-10-03 14:56:14]

PTAL. Since the checkers are now so simple (just a comparison), I've inlined
them for even greater savings! I promise I'll stop chasing this one soon and get
back to writing proper code! :)

[kwiberg-webrtc, 2016-10-03 17:31:34]

You could implement them with vector instructions for even more speed. Or maybe
use an FPGA.

Otherwise, this lgtm.

[ossu, 2016-10-04 08:39:46]

The CQ bit was checked by ossu@webrtc.org

[commit-bot: I haz the power, 2016-10-04 08:39:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-10-04 10:40:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-04 10:40:39]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[ossu, 2016-10-04 12:15:02]

Description was changed from

==========
Cache the subtype of each DecoderInfo to make the Is* checks quicker.

Addresses a regression in the NetEq performance test.

BUG=chromium:651426
==========

to

==========
Cache the subtype of each DecoderInfo to make the Is* checks quicker.

Addresses a regression in the NetEq performance test.

# Added NOTRY due to android_arm64_rel being swamped.
NOTRY=True
BUG=chromium:651426
==========

[ossu, 2016-10-04 12:15:12]

The CQ bit was checked by ossu@webrtc.org

[commit-bot: I haz the power, 2016-10-04 12:15:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-10-04 12:23:34]

Description was changed from

==========
Cache the subtype of each DecoderInfo to make the Is* checks quicker.

Addresses a regression in the NetEq performance test.

# Added NOTRY due to android_arm64_rel being swamped.
NOTRY=True
BUG=chromium:651426
==========

to

==========
Cache the subtype of each DecoderInfo to make the Is* checks quicker.

Addresses a regression in the NetEq performance test.

# Added NOTRY due to android_arm64_rel being swamped.
NOTRY=True
BUG=chromium:651426
==========

[commit-bot: I haz the power, 2016-10-04 12:23:35]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-04 12:23:40]

Description was changed from

==========
Cache the subtype of each DecoderInfo to make the Is* checks quicker.

Addresses a regression in the NetEq performance test.

# Added NOTRY due to android_arm64_rel being swamped.
NOTRY=True
BUG=chromium:651426
==========

to

==========
Cache the subtype of each DecoderInfo to make the Is* checks quicker.

Addresses a regression in the NetEq performance test.

# Added NOTRY due to android_arm64_rel being swamped.
NOTRY=True
BUG=chromium:651426

Committed: https://crrev.com/9f38c218ee0282042e5bc31d3d3a1b1f53e4be16
Cr-Commit-Position: refs/heads/master@{#14495}
==========

[commit-bot: I haz the power, 2016-10-04 12:23:41]

Patchset 4 (id:??) landed as
https://crrev.com/9f38c218ee0282042e5bc31d3d3a1b1f53e4be16
Cr-Commit-Position: refs/heads/master@{#14495}