Simplify locking in HRTFDatabaseLoader

Simplify locking in HRTFDatabaseLoader

The locks in loadTask() and loadAsynchronously() were blocking each
other for about 100 msec or more, causing the jank in the bug report.

Only the lock is loadTask() is needed because that's what writes the
m_hrtfDatabase pointer.  loadAsynchronously() doesn't need a lock because it
doesn't need to read m_hrtfDatabase.

Also simplified isLoaded() and database() which basically do the same
thing except for the return type.

database() (and, indirectly, isLoaded()) also gets a tryLock because
it is called from the audio thread which can't block.

BUG=602668
TEST=No jank in test in 602668

Committed: https://crrev.com/0494c3815a7eebceb73234e8da9e368e97fc1bfb
Cr-Commit-Position: refs/heads/master@{#406389}

[Raymond Toy, 2016-07-15 16:06:17]

The CQ bit was checked by rtoy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-15 16:06:44]

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

[commit-bot: I haz the power, 2016-07-15 16:16:36]

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

[commit-bot: I haz the power, 2016-07-15 16:16:37]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hongchan, 2016-07-15 16:19:00]

hongchan@chromium.org changed reviewers:
+ hongchan@chromium.org

[hongchan, 2016-07-15 16:19:00]

https://codereview.chromium.org/2154473003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/audio/HRTFDatabaseLoader.cpp (right):

https://codereview.chromium.org/2154473003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/audio/HRTFDatabaseLoader.cpp:79:
ASSERT(!isMainThread());
I don't mind changing this to DCHECK.

https://codereview.chromium.org/2154473003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/audio/HRTFDatabaseLoader.cpp:96: m_thread =
wrapUnique(Platform::current()->createThread("HRTF database loader"));
Is wrapUnique necessary here?

https://codereview.chromium.org/2154473003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/audio/HRTFDatabaseLoader.cpp:107:
MutexTryLocker tryLocker(m_lock);
How is this tryLocker is connected to the audio thread's lock? This lock is only
valid for the lock at line 83. No?

[Raymond Toy, 2016-07-15 16:27:07]

https://codereview.chromium.org/2154473003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/audio/HRTFDatabaseLoader.cpp (right):

https://codereview.chromium.org/2154473003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/audio/HRTFDatabaseLoader.cpp:96: m_thread =
wrapUnique(Platform::current()->createThread("HRTF database loader"));
On 2016/07/15 16:19:00, hoch wrote:
> Is wrapUnique necessary here?

Don't know. I didn't change this.

https://codereview.chromium.org/2154473003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/audio/HRTFDatabaseLoader.cpp:107:
MutexTryLocker tryLocker(m_lock);
On 2016/07/15 16:19:00, hoch wrote:
> How is this tryLocker is connected to the audio thread's lock? This lock is
only
> valid for the lock at line 83. No?

Yeah, this is for protecting access to m_hrtfDatabase, which is what we want to
protect.

[Raymond Toy, 2016-07-15 16:52:16]

The CQ bit was checked by rtoy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-15 16:52:38]

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

[hongchan, 2016-07-19 20:10:15]

lgtm

[Raymond Toy, 2016-07-19 20:20:05]

Description was changed from

==========
Simplify locking in HRTFDatabaseLoader

The locks in loadTask() and loadAsynchronously() were blocking each
other for about 0.5 sec, causing the jank in the bug report.

Only the lock is loadTask() is needed because that's what writes the
m_hrtfDatabase pointer.  loadAsynchronously() doesn't need a lock because it
doesn't need to read m_hrtfDatabase.

Also simplified isLoaded() and database() which basically do the same
thing except for the return type.

database() (and, indirectly, isLoaded()) also gets a tryLock because
it is called from the audio thread which can't block.

BUG=602668
TEST=No jank in test in 602668
==========

to

==========
Simplify locking in HRTFDatabaseLoader

The locks in loadTask() and loadAsynchronously() were blocking each
other for about 100 msec or more, causing the jank in the bug report.

Only the lock is loadTask() is needed because that's what writes the
m_hrtfDatabase pointer.  loadAsynchronously() doesn't need a lock because it
doesn't need to read m_hrtfDatabase.

Also simplified isLoaded() and database() which basically do the same
thing except for the return type.

database() (and, indirectly, isLoaded()) also gets a tryLock because
it is called from the audio thread which can't block.

BUG=602668
TEST=No jank in test in 602668
==========

[Raymond Toy, 2016-07-19 20:20:30]

The CQ bit was checked by rtoy@chromium.org

[commit-bot: I haz the power, 2016-07-19 20:21:05]

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

[commit-bot: I haz the power, 2016-07-19 21:45:07]

Description was changed from

==========
Simplify locking in HRTFDatabaseLoader

The locks in loadTask() and loadAsynchronously() were blocking each
other for about 100 msec or more, causing the jank in the bug report.

Only the lock is loadTask() is needed because that's what writes the
m_hrtfDatabase pointer.  loadAsynchronously() doesn't need a lock because it
doesn't need to read m_hrtfDatabase.

Also simplified isLoaded() and database() which basically do the same
thing except for the return type.

database() (and, indirectly, isLoaded()) also gets a tryLock because
it is called from the audio thread which can't block.

BUG=602668
TEST=No jank in test in 602668
==========

to

==========
Simplify locking in HRTFDatabaseLoader

The locks in loadTask() and loadAsynchronously() were blocking each
other for about 100 msec or more, causing the jank in the bug report.

Only the lock is loadTask() is needed because that's what writes the
m_hrtfDatabase pointer.  loadAsynchronously() doesn't need a lock because it
doesn't need to read m_hrtfDatabase.

Also simplified isLoaded() and database() which basically do the same
thing except for the return type.

database() (and, indirectly, isLoaded()) also gets a tryLock because
it is called from the audio thread which can't block.

BUG=602668
TEST=No jank in test in 602668
==========

[commit-bot: I haz the power, 2016-07-19 21:45:08]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-19 21:45:09]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-19 21:47:58]

Description was changed from

==========
Simplify locking in HRTFDatabaseLoader

The locks in loadTask() and loadAsynchronously() were blocking each
other for about 100 msec or more, causing the jank in the bug report.

Only the lock is loadTask() is needed because that's what writes the
m_hrtfDatabase pointer.  loadAsynchronously() doesn't need a lock because it
doesn't need to read m_hrtfDatabase.

Also simplified isLoaded() and database() which basically do the same
thing except for the return type.

database() (and, indirectly, isLoaded()) also gets a tryLock because
it is called from the audio thread which can't block.

BUG=602668
TEST=No jank in test in 602668
==========

to

==========
Simplify locking in HRTFDatabaseLoader

The locks in loadTask() and loadAsynchronously() were blocking each
other for about 100 msec or more, causing the jank in the bug report.

Only the lock is loadTask() is needed because that's what writes the
m_hrtfDatabase pointer.  loadAsynchronously() doesn't need a lock because it
doesn't need to read m_hrtfDatabase.

Also simplified isLoaded() and database() which basically do the same
thing except for the return type.

database() (and, indirectly, isLoaded()) also gets a tryLock because
it is called from the audio thread which can't block.

BUG=602668
TEST=No jank in test in 602668

Committed: https://crrev.com/0494c3815a7eebceb73234e8da9e368e97fc1bfb
Cr-Commit-Position: refs/heads/master@{#406389}
==========

[commit-bot: I haz the power, 2016-07-19 21:47:59]

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