[Sensors] Improvements in shared buffer managing

[Sensors] Improvements in shared buffer managing

This patch adds the following improvements:
-Introduces SeqLock synchronization for shared buffer reads and writes.

- Single sensor shared buffer is guaranteed to have same fixed size on every platform. This is important as our layout tests use fixed sized shared buffer on JS side.

- Introduced common data structures for sensor reading and sensor reading buffer that are used in both blink and platform layers.

For the implementation on Android shared buffer operations moved from java to generic C++ code, same code is to be reused for implementations on other platforms.

BUG=606766
Committed: https://crrev.com/9e8d58c66a5c826d4b517ecb8988508fc877ef39
Cr-Commit-Position: refs/heads/master@{#424403}

[Mikhail, 2016-10-06 12:06:26]

Description was changed from

==========
[Sensors] SeqLock synchronization for sensor reading buffer

This patch introduces SeqLock synchromization for shared buffer reads and
writes.

For the implementation on Android shared buffer operations moved from java to
generic C++ code, same code is to be reused for implementations on other
platforms.

BUG=606766
==========

to

==========
[Sensors] SeqLock synchronization for sensor reading buffer

This patch introduces SeqLock synchromization for shared buffer reads and
writes.

For the implementation on Android shared buffer operations moved from java to
generic C++ code, same code is to be reused for implementations on other
platforms.

BUG=606766
==========

[Mikhail, 2016-10-06 12:06:27]

mikhail.pozdnyakov@intel.com changed reviewers:
+ alexander.shalamov@intel.com

[shalamov, 2016-10-06 12:46:09]

https://codereview.chromium.org/2395853003/diff/1/device/generic_sensor/andro...
File
device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java
(right):

https://codereview.chromium.org/2395853003/diff/1/device/generic_sensor/andro...
device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java:16:
import java.nio.ByteBuffer;
not needed anymore.

https://codereview.chromium.org/2395853003/diff/1/device/generic_sensor/andro...
device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java:107:
private void fillSensorReadingData(SensorEvent event, ByteBuffer buffer)
not needed anymore.

https://codereview.chromium.org/2395853003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/sensor/ambient-light-sensor.html (right):

https://codereview.chromium.org/2395853003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/sensor/ambient-light-sensor.html:16: buffer[0] =
0.0; // Placeholder for seqlock.
Maybe better to make resetBuffer() function in sensor-helpers.js and then call
it after this.buffer_ = new Float64Array(this.buffer_array_);
and also reuse it in reset();

resetBuffer()
for (let i = 0; i < this.buffer_.length; ++i) {
          this.buffer_[i] = 0;
}
}

Then, you can remove buffer[0] = 0.0; // Placeholder for seqlock. and start
writing values starting from buffer[1]...

https://codereview.chromium.org/2395853003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/sensor/resources/sensor-helpers.js (right):

https://codereview.chromium.org/2395853003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/sensor/resources/sensor-helpers.js:178:
this.reading_size_in_bytes_ = 40; //
sizeof(device::SharedMemorySeqLockBuffer<device::mojom::blink::SensorReading>).
this.reading_size_in_bytes_ = (Object.keys(new sensor.SensorReading()).length +
1) * Float64Array.BYTES_PER_ELEMENT;

Object.keys(new sensor.SensorReading()).length - SensorReading field count
+ 1 // seq lock
Float64Array.BYTES_PER_ELEMENT // num of bytes per field

[shalamov, 2016-10-06 13:15:05]

nit: CL description synchromization => synchronization

[Mikhail, 2016-10-06 16:26:40]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 16:27:06]

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-10-06 16:37:53]

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

[commit-bot: I haz the power, 2016-10-06 16:37:54]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Andrew44e, 2016-10-07 08:20:38]

How to make resetBuffer() function in sensor-helpers.js? Is there a simple way
to do that? I guess I messed up the coding. That's not so easy, as I thought.
Mell - still working on http://domyhomeworkonline.net/ for Android

[Mikhail, 2016-10-07 09:05:24]

Description was changed from

==========
[Sensors] SeqLock synchronization for sensor reading buffer

This patch introduces SeqLock synchromization for shared buffer reads and
writes.

For the implementation on Android shared buffer operations moved from java to
generic C++ code, same code is to be reused for implementations on other
platforms.

BUG=606766
==========

to

==========
[Sensors] SeqLock synchronization for sensor reading buffer

This patch introduces SeqLock synchronization for shared buffer reads and
writes.

For the implementation on Android shared buffer operations moved from java to
generic C++ code, same code is to be reused for implementations on other
platforms.

BUG=606766
==========

[Mikhail, 2016-10-07 10:05:28]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-07 10:05:43]

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

[Mikhail, 2016-10-07 10:07:02]

Alex, thanks for commenting

https://codereview.chromium.org/2395853003/diff/1/device/generic_sensor/andro...
File
device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java
(right):

https://codereview.chromium.org/2395853003/diff/1/device/generic_sensor/andro...
device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java:16:
import java.nio.ByteBuffer;
On 2016/10/06 12:46:09, shalamov wrote:
> not needed anymore.

Done.

https://codereview.chromium.org/2395853003/diff/1/device/generic_sensor/andro...
device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java:107:
private void fillSensorReadingData(SensorEvent event, ByteBuffer buffer)
On 2016/10/06 12:46:09, shalamov wrote:
> not needed anymore.

Done.

https://codereview.chromium.org/2395853003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/sensor/ambient-light-sensor.html (right):

https://codereview.chromium.org/2395853003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/sensor/ambient-light-sensor.html:16: buffer[0] =
0.0; // Placeholder for seqlock.
On 2016/10/06 12:46:09, shalamov wrote:
> Maybe better to make resetBuffer() function in sensor-helpers.js and then call
> it after this.buffer_ = new Float64Array(this.buffer_array_);
> and also reuse it in reset();
> 
> resetBuffer()
> for (let i = 0; i < this.buffer_.length; ++i) {
>           this.buffer_[i] = 0;
> }
> }
> 
> Then, you can remove buffer[0] = 0.0; // Placeholder for seqlock. and start
> writing values starting from buffer[1]...

Done.

[commit-bot: I haz the power, 2016-10-07 10:14:27]

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

[commit-bot: I haz the power, 2016-10-07 10:14:28]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Mikhail, 2016-10-07 20:50:04]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-07 20:50:34]

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

[Mikhail, 2016-10-07 20:59:39]

Description was changed from

==========
[Sensors] SeqLock synchronization for sensor reading buffer

This patch introduces SeqLock synchronization for shared buffer reads and
writes.

For the implementation on Android shared buffer operations moved from java to
generic C++ code, same code is to be reused for implementations on other
platforms.

BUG=606766
==========

to

==========
[Sensors] Improvements in shared buffer managing

This patch adds the following improvements:
-Introduces SeqLock synchronization for shared buffer reads and writes.

- Single sensor shared buffer is guaranteed to have same fixed size on every
platform. This is important as our layout tests use fixed sized shared buffer on
JS side.

- Introduced common data structures for sensor reading and sensor reading buffer
that are used in both blink and platform layers. 

For the implementation on Android shared buffer operations moved from java to
generic C++ code, same code is to be reused for implementations on other
platforms.

BUG=606766
==========

[Mikhail, 2016-10-07 21:12:00]

mikhail.pozdnyakov@intel.com changed reviewers:
+ haraken@chromium.org, rockot@chromium.org, timvolodine@chromium.org,
tsepez@chromium.org

[Mikhail, 2016-10-07 21:12:45]

Please take a look.

[commit-bot: I haz the power, 2016-10-07 22:25:27]

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

[commit-bot: I haz the power, 2016-10-07 22:25:27]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-10-07 22:33:28]

mojom lgtm

[haraken, 2016-10-08 01:15:45]

modules/ LGTM on my side

[Mikhail, 2016-10-10 09:56:56]

Patchset #5 (id:80001) has been deleted

[Mikhail, 2016-10-10 09:58:10]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-10 09:58:19]

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

[shalamov, 2016-10-10 11:00:59]

lgtm

[commit-bot: I haz the power, 2016-10-10 12:06:36]

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

[commit-bot: I haz the power, 2016-10-10 12:06:37]

Dry run: This issue passed the CQ dry run.

[Mikhail, 2016-10-10 12:54:12]

mikhail.pozdnyakov@intel.com changed reviewers:
+ reillyg@chromium.org

[Mikhail, 2016-10-10 12:56:44]

Tim, Ken, Reilly, do you have any comments for  	device/generic_sensor/ changes?
Thanks.

[Ken Rockot(use gerrit already), 2016-10-10 16:02:09]

https://codereview.chromium.org/2395853003/diff/100001/device/generic_sensor/...
File device/generic_sensor/platform_sensor.cc (right):

https://codereview.chromium.org/2395853003/diff/100001/device/generic_sensor/...
device/generic_sensor/platform_sensor.cc:97: task_runner_->PostTask(
I believe this is incorrect. The PlatformSensor instance lives on thread A, and
presumably |task_runner_| can run tasks on some arbitrary other thread B.

|clients_| is owned by PlatformSensor and is not thread-safe, yet you're
accessing it on both threads.

WeakPtrs vended by WeakFactory must be dereferenced and invalidated on the same
thread. Since you only invalidate them implicitly in the destructor (on thread
A) it is not OK to dereference them on thread B.

https://codereview.chromium.org/2395853003/diff/100001/device/generic_sensor/...
File device/generic_sensor/platform_sensor_provider_base.cc (right):

https://codereview.chromium.org/2395853003/diff/100001/device/generic_sensor/...
device/generic_sensor/platform_sensor_provider_base.cc:112: uint64_t
PlatformSensorProviderBase::GetSharedBufferOffset(
nit: This doesn't need to be a member function at all, and maybe it should be a
helper function in the public client library? Presumably clients need to know
where to read the sensor data too. Could be in sensor_reading.h/cc?

https://codereview.chromium.org/2395853003/diff/100001/device/generic_sensor/...
File device/generic_sensor/public/cpp/sensor_reading.h (right):

https://codereview.chromium.org/2395853003/diff/100001/device/generic_sensor/...
device/generic_sensor/public/cpp/sensor_reading.h:17: static_assert(sizeof(Data)
<= sizeof(int64_t), "The field must be 64 bits.");
nit: Assertion is really that the field size must be <= 64 bits

https://codereview.chromium.org/2395853003/diff/100001/device/generic_sensor/...
device/generic_sensor/public/cpp/sensor_reading.h:31: Storage() { new (&value)
Data(); }
This is subtly broken if Data has a non-trivial destructor. Two decent options I
see:

 1. define ~Storage() { value.~Data(); }, or

 2. don't use a union at all. We can just do:

#pragma pack(push, 1)
  static_assert(sizeof(Data) <= sizeof(int64_t), "...");
  Data data_;
  uint8_t padding_[sizeof(int64_t) - sizeof(Data)];
#pragma pack(pop)
};


Also please use |name_| convention for private members.

https://codereview.chromium.org/2395853003/diff/100001/device/generic_sensor/...
File device/generic_sensor/public/interfaces/sensor_provider.mojom (right):

https://codereview.chromium.org/2395853003/diff/100001/device/generic_sensor/...
device/generic_sensor/public/interfaces/sensor_provider.mojom:23: // point
fields (pls see sensor_reading.h) and a seqlock, so its size is
nit: "please"

https://codereview.chromium.org/2395853003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right):

https://codereview.chromium.org/2395853003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:103: const int
kMaxReadAttemptsCount = 10;
What is the practical difference between trying 1 time vs trying 10 times vs
trying 100 times? Is this really what we want? What is the likelihood that the
attempt will fail once, twice, three times in a row? This just feels arbitrary
and fragile.

[Reilly Grant (use Gerrit), 2016-10-10 16:30:41]

https://codereview.chromium.org/2395853003/diff/100001/device/generic_sensor/...
File device/generic_sensor/platform_sensor.cc (right):

https://codereview.chromium.org/2395853003/diff/100001/device/generic_sensor/...
device/generic_sensor/platform_sensor.cc:97: task_runner_->PostTask(
On 2016/10/10 at 16:02:08, Ken Rockot wrote:
> I believe this is incorrect. The PlatformSensor instance lives on thread A,
and presumably |task_runner_| can run tasks on some arbitrary other thread B.
> 
> |clients_| is owned by PlatformSensor and is not thread-safe, yet you're
accessing it on both threads.
> 
> WeakPtrs vended by WeakFactory must be dereferenced and invalidated on the
same thread. Since you only invalidate them implicitly in the destructor (on
thread A) it is not OK to dereference them on thread B.

So far it looks like |task_runner_| is always the same thread the PlatformSensor
was constructed on but it took some digging to convince myself of that. If you
want to cache the result of base::ThreadTaskRunnerHandle::Get() then I suggest
doing it in the base PlatformSensor constructor instead of making readers check
that that is always what is being passed in in every subclass.

[Ken Rockot(use gerrit already), 2016-10-10 16:32:09]

On Mon, Oct 10, 2016 at 9:30 AM, <reillyg@chromium.org> wrote:

>
> https://codereview.chromium.org/2395853003/diff/100001/
> device/generic_sensor/platform_sensor.cc
> File device/generic_sensor/platform_sensor.cc (right):
>
> https://codereview.chromium.org/2395853003/diff/100001/
> device/generic_sensor/platform_sensor.cc#newcode97
> device/generic_sensor/platform_sensor.cc:97: task_runner_->PostTask(
> On 2016/10/10 at 16:02:08, Ken Rockot wrote:
> > I believe this is incorrect. The PlatformSensor instance lives on
> thread A, and presumably |task_runner_| can run tasks on some arbitrary
> other thread B.
> >
> > |clients_| is owned by PlatformSensor and is not thread-safe, yet
> you're accessing it on both threads.
> >
> > WeakPtrs vended by WeakFactory must be dereferenced and invalidated on
> the same thread. Since you only invalidate them implicitly in the
> destructor (on thread A) it is not OK to dereference them on thread B.
>
> So far it looks like |task_runner_| is always the same thread the
> PlatformSensor was constructed on but it took some digging to convince
> myself of that. If you want to cache the result of
> base::ThreadTaskRunnerHandle::Get() then I suggest doing it in the base
> PlatformSensor constructor instead of making readers check that that is
> always what is being passed in in every subclass.
>

In any case the documentation seems to indicate a clear intent to have the
task runner run tasks on a thread other than PlatformSensor's.

>
> https://codereview.chromium.org/2395853003/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-10-10 16:32:10]

On Mon, Oct 10, 2016 at 9:30 AM, <reillyg@chromium.org> wrote:

>
> https://codereview.chromium.org/2395853003/diff/100001/
> device/generic_sensor/platform_sensor.cc
> File device/generic_sensor/platform_sensor.cc (right):
>
> https://codereview.chromium.org/2395853003/diff/100001/
> device/generic_sensor/platform_sensor.cc#newcode97
> device/generic_sensor/platform_sensor.cc:97: task_runner_->PostTask(
> On 2016/10/10 at 16:02:08, Ken Rockot wrote:
> > I believe this is incorrect. The PlatformSensor instance lives on
> thread A, and presumably |task_runner_| can run tasks on some arbitrary
> other thread B.
> >
> > |clients_| is owned by PlatformSensor and is not thread-safe, yet
> you're accessing it on both threads.
> >
> > WeakPtrs vended by WeakFactory must be dereferenced and invalidated on
> the same thread. Since you only invalidate them implicitly in the
> destructor (on thread A) it is not OK to dereference them on thread B.
>
> So far it looks like |task_runner_| is always the same thread the
> PlatformSensor was constructed on but it took some digging to convince
> myself of that. If you want to cache the result of
> base::ThreadTaskRunnerHandle::Get() then I suggest doing it in the base
> PlatformSensor constructor instead of making readers check that that is
> always what is being passed in in every subclass.
>

In any case the documentation seems to indicate a clear intent to have the
task runner run tasks on a thread other than PlatformSensor's.

>
> https://codereview.chromium.org/2395853003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mikhail, 2016-10-10 18:27:07]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-10 18:27:19]

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

[Mikhail, 2016-10-10 18:29:20]

Ken, Reilly, thanks for review!

https://codereview.chromium.org/2395853003/diff/100001/device/generic_sensor/...
File device/generic_sensor/platform_sensor.cc (right):

https://codereview.chromium.org/2395853003/diff/100001/device/generic_sensor/...
device/generic_sensor/platform_sensor.cc:97: task_runner_->PostTask(
On 2016/10/10 16:30:41, Reilly Grant wrote:
> On 2016/10/10 at 16:02:08, Ken Rockot wrote:
> > I believe this is incorrect. The PlatformSensor instance lives on thread A,
> and presumably |task_runner_| can run tasks on some arbitrary other thread B.
> > 
> > |clients_| is owned by PlatformSensor and is not thread-safe, yet you're
> accessing it on both threads.
> > 
> > WeakPtrs vended by WeakFactory must be dereferenced and invalidated on the
> same thread. Since you only invalidate them implicitly in the destructor (on
> thread A) it is not OK to dereference them on thread B.
> 
> So far it looks like |task_runner_| is always the same thread the
PlatformSensor
> was constructed on but it took some digging to convince myself of that. If you
> want to cache the result of base::ThreadTaskRunnerHandle::Get() then I suggest
> doing it in the base PlatformSensor constructor instead of making readers
check
> that that is always what is being passed in in every subclass.
Right, |task_runner_| is PlatformSensor's native thread. However on many
platforms (Android, Mac, Linux) sensor data is obtained on a different thread,
so it's useful to have |task_runner_| for notifications from there.
I'll switch to initializing it in constructor.

https://codereview.chromium.org/2395853003/diff/100001/device/generic_sensor/...
File device/generic_sensor/platform_sensor_provider_base.cc (right):

https://codereview.chromium.org/2395853003/diff/100001/device/generic_sensor/...
device/generic_sensor/platform_sensor_provider_base.cc:112: uint64_t
PlatformSensorProviderBase::GetSharedBufferOffset(
On 2016/10/10 16:02:08, Ken Rockot wrote:
> nit: This doesn't need to be a member function at all, and maybe it should be
a
> helper function in the public client library? Presumably clients need to know
> where to read the sensor data too. Could be in sensor_reading.h/cc?

Thanks for hint! I've moved it here from sensor_impl.cc but sensor_reading.h is
a better place indeed.

https://codereview.chromium.org/2395853003/diff/100001/device/generic_sensor/...
File device/generic_sensor/public/cpp/sensor_reading.h (right):

https://codereview.chromium.org/2395853003/diff/100001/device/generic_sensor/...
device/generic_sensor/public/cpp/sensor_reading.h:17: static_assert(sizeof(Data)
<= sizeof(int64_t), "The field must be 64 bits.");
On 2016/10/10 16:02:08, Ken Rockot wrote:
> nit: Assertion is really that the field size must be <= 64 bits

Done.

https://codereview.chromium.org/2395853003/diff/100001/device/generic_sensor/...
device/generic_sensor/public/cpp/sensor_reading.h:31: Storage() { new (&value)
Data(); }
On 2016/10/10 16:02:08, Ken Rockot wrote:
> This is subtly broken if Data has a non-trivial destructor. Two decent options
I
> see:
> 
>  1. define ~Storage() { value.~Data(); }, or
> 
>  2. don't use a union at all. We can just do:
> 
> #pragma pack(push, 1)
>   static_assert(sizeof(Data) <= sizeof(int64_t), "...");
>   Data data_;
>   uint8_t padding_[sizeof(int64_t) - sizeof(Data)];
> #pragma pack(pop)
> };
> 
> 
> Also please use |name_| convention for private members.
Done. Thanks for catching this!

https://codereview.chromium.org/2395853003/diff/100001/device/generic_sensor/...
File device/generic_sensor/public/interfaces/sensor_provider.mojom (right):

https://codereview.chromium.org/2395853003/diff/100001/device/generic_sensor/...
device/generic_sensor/public/interfaces/sensor_provider.mojom:23: // point
fields (pls see sensor_reading.h) and a seqlock, so its size is
On 2016/10/10 16:02:08, Ken Rockot wrote:
> nit: "please"

Done.

https://codereview.chromium.org/2395853003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProxy.cpp (right):

https://codereview.chromium.org/2395853003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:103: const int
kMaxReadAttemptsCount = 10;
On 2016/10/10 16:02:08, Ken Rockot wrote:
> What is the practical difference between trying 1 time vs trying 10 times vs
> trying 100 times? Is this really what we want? What is the likelihood that the
> attempt will fail once, twice, three times in a row? This just feels arbitrary
> and fragile.

I do not think there is a significant likelihood of failing even once: even when
writes and reads are processed with the same frequency, there is always a lag
between 2 timers (on platform side and in render process) due to IPC mojo calls
so they won't actually overlap.

Assuming that writing takes approximately same amount of asm instructions (x) as
reading, 10*x times should be enough to distinguish between an accidental
failure and a fatal error.

[commit-bot: I haz the power, 2016-10-10 18:32:58]

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

[commit-bot: I haz the power, 2016-10-10 18:32:59]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Mikhail, 2016-10-10 18:41:55]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

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

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-10-10 18:55:58]

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

[commit-bot: I haz the power, 2016-10-10 18:55:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ken Rockot(use gerrit already), 2016-10-10 19:12:43]

Ah thanks for the explanation on task runner usage, makes sense.

However, I am still not happy about this pattern:

  size_t keepTrying = kArbitraryNumber;
  while (!trySomething() && keepTrying)
    --keepTrying;

This seems fragile and error prone, and it is not a pattern you find elsewhere
in chromium code.  Surely we can do something better than this.

https://codereview.chromium.org/2395853003/diff/140001/device/generic_sensor/...
File device/generic_sensor/platform_sensor.h (right):

https://codereview.chromium.org/2395853003/diff/140001/device/generic_sensor/...
device/generic_sensor/platform_sensor.h:86: mojo::ScopedSharedBufferMapping
shared_buffer_mapping_;
This must be const if you want UpdateSensorReading to be thread-safe.

Otherwise there's nothing to stop subclasses (or even PlatformSensor itself)
from accidentally modifying it and creating data races in UpdateSensorReading.

[Mikhail, 2016-10-10 20:03:17]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-10 20:03:43]

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

[Mikhail, 2016-10-10 20:05:15]

On 2016/10/10 19:12:43, Ken Rockot wrote:
> Ah thanks for the explanation on task runner usage, makes sense.
> 
> However, I am still not happy about this pattern:
> 
>   size_t keepTrying = kArbitraryNumber;
>   while (!trySomething() && keepTrying)
>     --keepTrying;
> 
> This seems fragile and error prone, and it is not a pattern you find elsewhere
> in chromium code.  Surely we can do something better than this.

The reference doc for seqlock
(http://www.concurrencykit.org/doc/ck_sequence.html) recommends just:

   do {
        version = ck_sequence_read_begin(&seqlock);
                           copy = global;
    } while (ck_sequence_read_retry(&seqlock, version));

but I'm afraid doing this we can end up with a dead-loop in renderer.

The 'content::SharedMemorySeqLockReader' in Chromium is doing the same as the
code in this CL: it also uses '10' as maximum amount of read attempts.
Please look at
https://chromium.googlesource.com/chromium/src.git/+/lkcr/content/renderer/sh...

I'm not sure if we could find a better approach for this sync mechanism.
> 
>
https://codereview.chromium.org/2395853003/diff/140001/device/generic_sensor/...
> File device/generic_sensor/platform_sensor.h (right):
> 
>
https://codereview.chromium.org/2395853003/diff/140001/device/generic_sensor/...
> device/generic_sensor/platform_sensor.h:86: mojo::ScopedSharedBufferMapping
> shared_buffer_mapping_;
> This must be const if you want UpdateSensorReading to be thread-safe.
> 
> Otherwise there's nothing to stop subclasses (or even PlatformSensor itself)
> from accidentally modifying it and creating data races in UpdateSensorReading.
Done.

[Ken Rockot(use gerrit already), 2016-10-10 20:16:03]

Well, I won't block this CL on that discussion, so LGTM.

I think that either:

1. Something is fundamentally broken if this would actually get stuck in an
endless loop, so we should just get rid of the counter and loop indefinitely -
i.e. if this actually breaks, a renderer hang would seem better than a flaky API

or

2. we should reconsider the use of a se

[Ken Rockot(use gerrit already), 2016-10-10 20:16:41]

On 2016/10/10 at 20:16:03, Ken Rockot wrote:
> Well, I won't block this CL on that discussion, so LGTM.
> 
> I think that either:
> 
> 1. Something is fundamentally broken if this would actually get stuck in an
endless loop, so we should just get rid of the counter and loop indefinitely -
i.e. if this actually breaks, a renderer hang would seem better than a flaky API
> 
> or
> 
> 2. we should reconsider the use of a se

Oops, pressed send too early...

2. We should reconsider the use of a seqlock here and instead use something
which doesn't require this sort of behavior

[Reilly Grant (use Gerrit), 2016-10-10 21:02:36]

lgtm

[commit-bot: I haz the power, 2016-10-10 21:32:49]

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

[commit-bot: I haz the power, 2016-10-10 21:32:50]

Dry run: This issue passed the CQ dry run.

[Mikhail, 2016-10-11 09:13:40]

The CQ bit was checked by mikhail.pozdnyakov@intel.com

[Mikhail, 2016-10-11 09:13:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, tsepez@chromium.org, alexander.shalamov@intel.com
Link to the patchset: https://codereview.chromium.org/2395853003/#ps160001
(title: "Test compilation fix + comment from Ken")

[commit-bot: I haz the power, 2016-10-11 09:13:54]

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

[Mikhail, 2016-10-11 09:17:16]

The CQ bit was unchecked by mikhail.pozdnyakov@intel.com

[Mikhail, 2016-10-11 10:56:56]

The CQ bit was unchecked by mikhail.pozdnyakov@intel.com

[Mikhail, 2016-10-11 10:56:57]

The CQ bit was checked by mikhail.pozdnyakov@intel.com

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

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-10-11 11:02:27]

Description was changed from

==========
[Sensors] Improvements in shared buffer managing

This patch adds the following improvements:
-Introduces SeqLock synchronization for shared buffer reads and writes.

- Single sensor shared buffer is guaranteed to have same fixed size on every
platform. This is important as our layout tests use fixed sized shared buffer on
JS side.

- Introduced common data structures for sensor reading and sensor reading buffer
that are used in both blink and platform layers. 

For the implementation on Android shared buffer operations moved from java to
generic C++ code, same code is to be reused for implementations on other
platforms.

BUG=606766
==========

to

==========
[Sensors] Improvements in shared buffer managing

This patch adds the following improvements:
-Introduces SeqLock synchronization for shared buffer reads and writes.

- Single sensor shared buffer is guaranteed to have same fixed size on every
platform. This is important as our layout tests use fixed sized shared buffer on
JS side.

- Introduced common data structures for sensor reading and sensor reading buffer
that are used in both blink and platform layers. 

For the implementation on Android shared buffer operations moved from java to
generic C++ code, same code is to be reused for implementations on other
platforms.

BUG=606766
==========

[commit-bot: I haz the power, 2016-10-11 11:02:29]

Committed patchset #8 (id:160001)

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

Description was changed from

==========
[Sensors] Improvements in shared buffer managing

This patch adds the following improvements:
-Introduces SeqLock synchronization for shared buffer reads and writes.

- Single sensor shared buffer is guaranteed to have same fixed size on every
platform. This is important as our layout tests use fixed sized shared buffer on
JS side.

- Introduced common data structures for sensor reading and sensor reading buffer
that are used in both blink and platform layers. 

For the implementation on Android shared buffer operations moved from java to
generic C++ code, same code is to be reused for implementations on other
platforms.

BUG=606766
==========

to

==========
[Sensors] Improvements in shared buffer managing

This patch adds the following improvements:
-Introduces SeqLock synchronization for shared buffer reads and writes.

- Single sensor shared buffer is guaranteed to have same fixed size on every
platform. This is important as our layout tests use fixed sized shared buffer on
JS side.

- Introduced common data structures for sensor reading and sensor reading buffer
that are used in both blink and platform layers.

For the implementation on Android shared buffer operations moved from java to
generic C++ code, same code is to be reused for implementations on other
platforms.

BUG=606766

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

[commit-bot: I haz the power, 2016-10-11 11:04:06]

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