Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1864323002:
Migrate from CRITICAL_SECTION to SRWLOCK (Closed)
DescriptionMigrate from CRITICAL_SECTION to SRWLOCK
With the new Win 7 minimum version requirement, we can start using SRWLOCKs, which are lighter weight and generally faster than CRITICAL_SECTIONS under exclusive mode, which is what we use.
BUG=592752
Committed: https://crrev.com/7a8a904bfdf23bb8cacba471718bc09a0d9a3175
Cr-Commit-Position: refs/heads/master@{#386442}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Update Comment #
Depends on Patchset: Messages
Total messages: 38 (17 generated)
Description was changed from ========== Migrate from CRITICAL_SECTION to SRWLOCK BUG=592752 ========== to ========== Migrate from CRITICAL_SECTION to SRWLOCK With the new Win 7 minimum version requirement, we can start using SRWLOCKs, which are lighter weight and generally faster than CRITICAL_SECTIONS under exclusive mode, which is what we use. BUG=592752 ==========
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1864323002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1864323002/1
robliao@chromium.org changed reviewers: + danakj@chromium.org, scottmg@chromium.org
danakj: Please provide OWNER approval for this changelist. Thanks! scottmg: FYI, feel free to take a look! Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1864323002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1864323002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
scottmg@chromium.org changed reviewers: + jschuh@chromium.org
+jschuh who was timing swrlocks yesterday (I assume with you, but just in case not or he has some thoughts) https://codereview.chromium.org/1864323002/diff/1/base/synchronization/lock_i... File base/synchronization/lock_impl_win.cc (right): https://codereview.chromium.org/1864323002/diff/1/base/synchronization/lock_i... base/synchronization/lock_impl_win.cc:19: ::AcquireSRWLockExclusive(&native_handle_); SRWs aren't recursive where CSs are, right? Is this going to magically be OK?
https://codereview.chromium.org/1864323002/diff/1/base/synchronization/lock_i... File base/synchronization/lock_impl_win.cc (right): https://codereview.chromium.org/1864323002/diff/1/base/synchronization/lock_i... base/synchronization/lock_impl_win.cc:19: ::AcquireSRWLockExclusive(&native_handle_); On 2016/04/06 22:36:12, scottmg wrote: > SRWs aren't recursive where CSs are, right? Is this going to magically be OK? Yup, because we don't permit recursive Locks. See https://code.google.com/p/chromium/codesearch#chromium/src/base/synchronizati...
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1864323002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1864323002/1
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1864323002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1864323002/20001
> which are lighter weight and generally faster than CRITICAL_SECTIONS under exclusive mode, which is what we use. Do you have some numbers/
On 2016/04/06 23:32:27, danakj wrote:
> > which are lighter weight and generally faster than CRITICAL_SECTIONS under
> exclusive mode, which is what we use.
>
> Do you have some numbers/
Yup, I ran tightloop tests referenced in the bug.
Tightloop Tests on a Powerful Machine
for (unsigned int i = 0; i < 10000000; ++i) {
EnterCriticalSection(&critsec);
LeaveCriticalSection(&critsec);
}
vs
for (unsigned int i = 0; i < 10000000; i++) {
AcquireSRWLockExclusive(&srwlock);
ReleaseSRWLockExclusive(&srwlock);
}
The SRWLock version for release builds is around 20-30 ms faster.
On debug builds, SRWLock is around 40-50 ms faster.
Other folks have found similar gains:
http://nasutechtips.blogspot.com/2010/11/slim-read-write-srw-locks.html
Microsoft claims they're optimized for speed and space
(https://msdn.microsoft.com/en-us/library/aa904937(v=VS.85).aspx).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Cool, thanks! LGTM
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1864323002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1864323002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/04/07 19:14:29, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. From the win perfbot, we shouldn't see any major performance regression with this change. If anything, we might see a modest improvement. page_cycler.typical_25:warm_times.page_load_time 992.18ms +/- 12.75% SRWLock 1005.07ms +/- 12.55% Critical Section page_cycler.typical_25:cold_times.page_load_time 1331.29ms +/- 12.53% SRWLock 1349.85ms +/- 12.55% Critical Section http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-04... Given that this is a low level change, I'll hold off past branch point before sending this through.
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1864323002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1864323002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robliao@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1864323002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1864323002/20001
Message was sent while issue was closed.
Description was changed from ========== Migrate from CRITICAL_SECTION to SRWLOCK With the new Win 7 minimum version requirement, we can start using SRWLOCKs, which are lighter weight and generally faster than CRITICAL_SECTIONS under exclusive mode, which is what we use. BUG=592752 ========== to ========== Migrate from CRITICAL_SECTION to SRWLOCK With the new Win 7 minimum version requirement, we can start using SRWLOCKs, which are lighter weight and generally faster than CRITICAL_SECTIONS under exclusive mode, which is what we use. BUG=592752 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Migrate from CRITICAL_SECTION to SRWLOCK With the new Win 7 minimum version requirement, we can start using SRWLOCKs, which are lighter weight and generally faster than CRITICAL_SECTIONS under exclusive mode, which is what we use. BUG=592752 ========== to ========== Migrate from CRITICAL_SECTION to SRWLOCK With the new Win 7 minimum version requirement, we can start using SRWLOCKs, which are lighter weight and generally faster than CRITICAL_SECTIONS under exclusive mode, which is what we use. BUG=592752 Committed: https://crrev.com/7a8a904bfdf23bb8cacba471718bc09a0d9a3175 Cr-Commit-Position: refs/heads/master@{#386442} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/7a8a904bfdf23bb8cacba471718bc09a0d9a3175 Cr-Commit-Position: refs/heads/master@{#386442} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
