Removed unnecessary potential locking

Removed unnecessary potential locking

Function isInitialized, unlike calls ensureinitialized should not block
the thread, but it will if initialization is in progress at this moment.
It is not necessery to synchronize the sInitialized flag in
isInitialized function. It is enough that flag have a primitive type and
has a volatile qualifier.

R=nyquist@chromium.org, rmcilroy@chromium.org
TEST=It is rather hard to test. We've found ~300ms stuck on UI thread,
on calling LibraryLoader.isInitialized from ContentApplication.onCreate
with profiler

Committed: https://crrev.com/e1d5ac1d7c0a594f0c218c7031095ad684cc56df
Cr-Commit-Position: refs/heads/master@{#318208}

[rmcilroy, 2015-02-12 17:31:26]

rmcilroy@chromium.org changed reviewers:
+ simonb@chromium.org

[rmcilroy, 2015-02-12 17:31:27]

+simonb who knows this code better than me.

[nyquist, 2015-02-18 19:44:42]

lgtm

[simonb (inactive), 2015-02-19 11:07:29]

lgtm

[ripp, 2015-02-19 11:10:20]

The CQ bit was checked by ripp@yandex-team.ru

[commit-bot: I haz the power, 2015-02-19 11:11:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/925513002/1

[commit-bot: I haz the power, 2015-02-19 11:13:54]

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

[commit-bot: I haz the power, 2015-02-19 11:13:55]

Try jobs failed on following builders:
  ios_rel_device_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)

[ripp, 2015-02-20 07:21:09]

New patchsets have been uploaded after l-g-t-m from
nyquist@chromium.org,simonb@chromium.org

[ripp, 2015-02-20 07:23:08]

LibraryLoader now become singleton. Since it is never destroyed it is safe to
make it volatile and do not synchronize read access to it

[rmcilroy, 2015-02-20 10:43:09]

On 2015/02/20 07:23:08, ripp wrote:
> LibraryLoader now become singleton. Since it is never destroyed it is safe to
> make it volatile and do not synchronize read access to it

Not LGTM on patch set #2.  mLibraryProcessType isn't volatile and needs to be
protected by a lock.  Having looked through the code it looks like we don't call
getLibraryProcessType() other than from the JNI glue function
library_loader_hooks.cc::GetLibraryProcessType() - which is itself not called
AFAIKT.  Could we just remove this function and the corresponding function in
library_loader_hooks.cc?

[ripp, 2015-02-20 11:22:30]

On 2015/02/20 10:43:09, rmcilroy wrote:
> On 2015/02/20 07:23:08, ripp wrote:
> > LibraryLoader now become singleton. Since it is never destroyed it is safe
to
> > make it volatile and do not synchronize read access to it
> 
> Not LGTM on patch set #2.  mLibraryProcessType isn't volatile and needs to be
> protected by a lock.

mLibraryProcessType is initialized once in LibraryLoader's constructor. It can't
be accessed for read and write simultaneously. Constructor itself is
lock-protected (instantiating LibraryLoader under sLock), what means that
changes will be visible to all threads after leaving synchronized section.

[rmcilroy, 2015-02-20 11:39:15]

On 2015/02/20 11:22:30, ripp wrote:
> On 2015/02/20 10:43:09, rmcilroy wrote:
> > On 2015/02/20 07:23:08, ripp wrote:
> > > LibraryLoader now become singleton. Since it is never destroyed it is safe
> to
> > > make it volatile and do not synchronize read access to it
> > 
> > Not LGTM on patch set #2.  mLibraryProcessType isn't volatile and needs to
be
> > protected by a lock.
> 
> mLibraryProcessType is initialized once in LibraryLoader's constructor. It
can't
> be accessed for read and write simultaneously. Constructor itself is
> lock-protected (instantiating LibraryLoader under sLock), what means that
> changes will be visible to all threads after leaving synchronized section.

This is the case now, but it may not be in the future and it would be
non-obvious to anyone who modifies this class in the future.

In anycase, as I mentioned above you are micro-optimizing a method which is
never called elsewhere in the code, so I would rather just see the method
removed entirely.

[ripp, 2015-02-20 12:39:00]

Ok, I've removed GetLibraryProcessType

[ripp, 2015-02-20 12:41:39]

Sorry about two last patch sets - too carried away :)

[rmcilroy, 2015-02-20 17:35:39]

rmcilroy@chromium.org changed reviewers:
+ michaelbai@chromium.org, torne@chromium.org

[rmcilroy, 2015-02-20 17:35:40]

Sorry, I just noticed that the GetLibraryProcessType() was only add a week ago. 
Michael / Torne: are we going to use this new method eventually or is something
that has been left over which we can delete?

> Sorry about two last patch sets - too carried away :)

Yeah no problem, thanks for the contribution :).

[michaelbai, 2015-02-20 18:10:12]

On 2015/02/20 17:35:40, rmcilroy wrote:
> Sorry, I just noticed that the GetLibraryProcessType() was only add a week
ago. 
> Michael / Torne: are we going to use this new method eventually or is
something
> that has been left over which we can delete?
> 
> > Sorry about two last patch sets - too carried away :)
> 
> Yeah no problem, thanks for the contribution :).

The method will be used, please keep it.

[michaelbai, 2015-02-20 18:10:55]

On 2015/02/20 17:35:40, rmcilroy wrote:
> Sorry, I just noticed that the GetLibraryProcessType() was only add a week
ago. 
> Michael / Torne: are we going to use this new method eventually or is
something
> that has been left over which we can delete?
> 
> > Sorry about two last patch sets - too carried away :)
> 
> Yeah no problem, thanks for the contribution :).

The method will be used, please keep it.

[ripp, 2015-02-24 11:58:23]

I've restored GetLibraryProcessType and made mLibraryProcessType volatile

[michaelbai, 2015-02-25 04:57:12]

https://codereview.chromium.org/925513002/diff/100001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/925513002/diff/100001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:84:
private volatile int mLibraryProcessType;
You probably should use final instead of volatile because it isn't expected to
be changed after object created.

[ripp, 2015-02-25 08:32:23]

https://codereview.chromium.org/925513002/diff/100001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/925513002/diff/100001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:84:
private volatile int mLibraryProcessType;
On 2015/02/25 04:57:12, michaelbai wrote:
> You probably should use final instead of volatile because it isn't expected to
> be changed after object created.

Done.

[rmcilroy, 2015-02-25 14:10:20]

lgtm once comments are added.

https://codereview.chromium.org/925513002/diff/120001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/925513002/diff/120001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:57:
private volatile boolean mInitialized;
please add a comment stating that this field is accessed without a lock and thus
should remain a one-way switch.

https://codereview.chromium.org/925513002/diff/120001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:84:
private final int mLibraryProcessType;
please add a comment here stating why this needs to stay final

[ripp, 2015-02-25 15:36:26]

https://codereview.chromium.org/925513002/diff/120001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/925513002/diff/120001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:57:
private volatile boolean mInitialized;
On 2015/02/25 14:10:20, rmcilroy wrote:
> please add a comment stating that this field is accessed without a lock and
thus
> should remain a one-way switch.

Done.

https://codereview.chromium.org/925513002/diff/120001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:84:
private final int mLibraryProcessType;
On 2015/02/25 14:10:20, rmcilroy wrote:
> please add a comment here stating why this needs to stay final

Done.

[ripp, 2015-02-26 07:12:10]

The CQ bit was checked by ripp@yandex-team.ru

[ripp, 2015-02-26 07:12:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, simonb@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/925513002/#ps140001
(title: "Comments added")

[commit-bot: I haz the power, 2015-02-26 07:12:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/925513002/140001

[commit-bot: I haz the power, 2015-02-26 08:07:57]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-02-26 08:08:44]

Patchset 8 (id:??) landed as
https://crrev.com/e1d5ac1d7c0a594f0c218c7031095ad684cc56df
Cr-Commit-Position: refs/heads/master@{#318208}