Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(154)

Issue 2667513003: Remove some LazyInstance use in base/ (Closed)

Created:
2 years, 10 months ago by scottmg
Modified:
2 years, 10 months ago
CC:
chromium-reviews, grt+watch_chromium.org, wfh+watch_chromium.org, sadrul, gavinp+memory_chromium.org, vmpstr+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Patch Set 1 #

Patch Set 2 : . #

Patch Set 3 : . #

Patch Set 4 : . #

Patch Set 5 : . #

Patch Set 6 : . #

Patch Set 7 : . #

Total comments: 1

Patch Set 8 : iwyu #

Patch Set 9 : iwyu2 #

Patch Set 10 : iwyu3 #

Patch Set 11 : improve cpu.cc #

Patch Set 12 : . #

Patch Set 13 : . #

Patch Set 14 : . #

Total comments: 6

Patch Set 15 : fixes #

Total comments: 2

Patch Set 16 : . #

Total comments: 1

Patch Set 17 : no message_window #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+120 lines, -165 lines) Patch
M base/cpu.cc View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +10 lines, -19 lines 0 comments Download
M base/debug/close_handle_hook_win.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +0 lines, -3 lines 0 comments Download
M base/debug/close_handle_hook_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 5 chunks +1 line, -17 lines 0 comments Download
M base/memory/memory_pressure_listener.cc View 1 5 chunks +8 lines, -7 lines 0 comments Download
M base/message_loop/message_loop.cc View 1 5 chunks +8 lines, -8 lines 0 comments Download
M base/path_service.cc View 2 chunks +2 lines, -4 lines 0 comments Download
M base/threading/thread_local_storage.cc View 1 2 3 5 chunks +9 lines, -7 lines 0 comments Download
M base/threading/watchdog.cc View 1 2 4 chunks +6 lines, -4 lines 0 comments Download
M base/threading/worker_pool.cc View 1 2 3 3 chunks +2 lines, -5 lines 0 comments Download
M base/threading/worker_pool_win.cc View 1 2 2 chunks +7 lines, -5 lines 0 comments Download
M base/time/time_posix.cc View 1 6 chunks +8 lines, -7 lines 0 comments Download
M base/time/time_win.cc View 1 5 chunks +7 lines, -6 lines 0 comments Download
M base/trace_event/trace_log.cc View 1 2 3 8 chunks +14 lines, -19 lines 1 comment Download
M base/win/scoped_handle.cc View 1 4 chunks +8 lines, -8 lines 0 comments Download
M base/win/win_util.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +26 lines, -42 lines 0 comments Download
M chrome/app/chrome_main_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +0 lines, -4 lines 0 comments Download
M components/guest_view/renderer/guest_view_container.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/frame_host/frame_tree_node.cc View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M content/utility/in_process_utility_thread.cc View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M storage/browser/quota/quota_manager.cc View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download

Depends on Patchset:

Dependent Patchsets:

Messages

Total messages: 90 (70 generated)
Mark Mentovai
I know you didn’t request review, I’m just commenting because you linked to this from ...
2 years, 10 months ago (2017-01-31 00:52:09 UTC) #26
scottmg
On 2017/01/31 00:52:09, Mark Mentovai wrote: > I know you didn’t request review, I’m just ...
2 years, 10 months ago (2017-01-31 01:03:05 UTC) #27
scottmg
Updated cpu.cc to match one of your suggestions (inline in the one use seemed too ...
2 years, 10 months ago (2017-01-31 05:00:59 UTC) #48
Mark Mentovai
LGTM https://codereview.chromium.org/2667513003/diff/260001/base/debug/close_handle_hook_win.cc File base/debug/close_handle_hook_win.cc (left): https://codereview.chromium.org/2667513003/diff/260001/base/debug/close_handle_hook_win.cc#oldcode206 base/debug/close_handle_hook_win.cc:206: base::LazyInstance<HandleHooks> g_hooks = LAZY_INSTANCE_INITIALIZER; This one wasn’t “leaky”, ...
2 years, 10 months ago (2017-01-31 14:37:01 UTC) #51
scottmg
Thanks. https://codereview.chromium.org/2667513003/diff/260001/base/debug/close_handle_hook_win.cc File base/debug/close_handle_hook_win.cc (left): https://codereview.chromium.org/2667513003/diff/260001/base/debug/close_handle_hook_win.cc#oldcode206 base/debug/close_handle_hook_win.cc:206: base::LazyInstance<HandleHooks> g_hooks = LAZY_INSTANCE_INITIALIZER; On 2017/01/31 14:37:01, Mark ...
2 years, 10 months ago (2017-01-31 18:19:16 UTC) #54
Mark Mentovai
LGTM https://codereview.chromium.org/2667513003/diff/280001/base/debug/close_handle_hook_win.cc File base/debug/close_handle_hook_win.cc (right): https://codereview.chromium.org/2667513003/diff/280001/base/debug/close_handle_hook_win.cc#newcode233 base/debug/close_handle_hook_win.cc:233: void HandleHooks::Unpatch() { Should we lose this dead ...
2 years, 10 months ago (2017-01-31 18:25:26 UTC) #57
scottmg
(Also fixed ~WindowClass() call). TBR jam for #include fixes outside of base/. https://codereview.chromium.org/2667513003/diff/280001/base/debug/close_handle_hook_win.cc File base/debug/close_handle_hook_win.cc ...
2 years, 10 months ago (2017-01-31 18:42:02 UTC) #60
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2667513003/300001
2 years, 10 months ago (2017-01-31 18:43:56 UTC) #64
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/373945)
2 years, 10 months ago (2017-01-31 19:58:37 UTC) #66
scottmg
https://codereview.chromium.org/2667513003/diff/300001/base/win/message_window.cc File base/win/message_window.cc (right): https://codereview.chromium.org/2667513003/diff/300001/base/win/message_window.cc#newcode41 base/win/message_window.cc:41: AtExitManager::RegisterCallback(deleter, ret); This "quick fix" is incorrect because in ...
2 years, 10 months ago (2017-01-31 21:27:43 UTC) #67
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2667513003/320001
2 years, 10 months ago (2017-01-31 21:57:08 UTC) #73
commit-bot: I haz the power
Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isolation/builds/5772)
2 years, 10 months ago (2017-02-01 00:00:19 UTC) #75
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2667513003/320001
2 years, 10 months ago (2017-02-01 00:08:29 UTC) #77
jam
lgtm
2 years, 10 months ago (2017-02-01 01:13:43 UTC) #78
commit-bot: I haz the power
Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isolation/builds/5781)
2 years, 10 months ago (2017-02-01 02:41:55 UTC) #80
scottmg
On 2017/02/01 02:41:55, commit-bot: I haz the power wrote: > Try jobs failed on following ...
2 years, 10 months ago (2017-02-01 18:16:53 UTC) #82
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2667513003/320001
2 years, 10 months ago (2017-02-01 18:18:18 UTC) #84
commit-bot: I haz the power
Committed patchset #17 (id:320001) as https://chromium.googlesource.com/chromium/src/+/6ece5aec616aa4d8d06359e426ac154904aabe5b
2 years, 10 months ago (2017-02-01 18:26:05 UTC) #87
Primiano Tucci (use gerrit)
https://codereview.chromium.org/2667513003/diff/320001/base/trace_event/trace_log.cc File base/trace_event/trace_log.cc (right): https://codereview.chromium.org/2667513003/diff/320001/base/trace_event/trace_log.cc#newcode92 base/trace_event/trace_log.cc:92: static auto filters = new std::vector<std::unique_ptr<TraceEventFilter>>(); Nice, this one ...
2 years, 10 months ago (2017-02-07 16:20:36 UTC) #89
scottmg
2 years, 10 months ago (2017-02-07 18:11:35 UTC) #90
Message was sent while issue was closed.
On 2017/02/07 16:20:36, Primiano Tucci wrote:
>
https://codereview.chromium.org/2667513003/diff/320001/base/trace_event/trace...
> File base/trace_event/trace_log.cc (right):
> 
>
https://codereview.chromium.org/2667513003/diff/320001/base/trace_event/trace...
> base/trace_event/trace_log.cc:92: static auto filters = new
> std::vector<std::unique_ptr<TraceEventFilter>>();
> Nice, this one seems to have caused a 5% improvement of tracing macros in
linux
> (which is good)
>
https://chromeperf.appspot.com/report?sid=a9893b9f1441204d5064113eb8e6a806cf3...
> 
> I guess this mostly because of getting read of the acquire_load used by
> lazy_instance and instead is performing an efficient thread-safe
initialization
> using some compiler builtin.
> (Note: acquire-load on x86 should be "just a load" [1]. However, as I found
out
> in crbug.com/593344 today two full mfences are emitted because of
> crbug.com/592903). Which also would explain why this improvement showed up
only
> on linux.
> 
> [1] https://www.cl.cam.ac.uk/~pes20/cpp/cpp0xmappings.html

Interesting, thanks for investigating.

Powered by Google App Engine
This is Rietveld 408576698