New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 747657 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug



Sign in to add a comment

DiscardableSharedMemoryManagerTests log errors, and are flakey

Project Member Reported by w...@chromium.org, Jul 22 2017

Issue description

Running the DiscardableSharedMemoryManager tests locally in a Debug build shows two issues:

DiscardableSharedMemoryManagerTest.Purge and
DiscardableSharedMemoryManagerTest.EnforceMemoryPolicy both log an error de-committing pages:

[13528:34572:0721/232351.356:2379010265:ERROR:discardable_shared_memory.cc(387)] VirtualFree() MEM_DECOMMIT failed in Purge(): The parameter is incorrect. (0x57)

Additionally, if you run the tests several times you will generally see at least one test fail with a stack like:

[21332:22492:0721/232412.813:2379031718:FATAL:observer_list_threadsafe.h(90)] Check failed: !ContainsKey(observers_, observer).
Backtrace:
        base::debug::StackTrace::StackTrace [0x00000001800D48A5+69] (c:\src\git-chrome\src\base\debug\stack_trace_win.cc:217)
        base::debug::StackTrace::StackTrace [0x00000001800D43C8+24] (c:\src\git-chrome\src\base\debug\stack_trace.cc:199)
        logging::LogMessage::~LogMessage [0x0000000180147270+112] (c:\src\git-chrome\src\base\logging.cc:554)
        base::ObserverListThreadSafe<base::MemoryCoordinatorClient>::AddObserver [0x000000018015D5D0+224] (c:\src\git-chrome\src\base\observer_list_threadsafe.h:92)
        base::MemoryCoordinatorClientRegistry::Register [0x000000018015DDD8+40] (c:\src\git-chrome\src\base\memory\memory_coordinator_client_registry.cc:25)
        discardable_memory::DiscardableSharedMemoryManager::DiscardableSharedMemoryManager [0x0000000019961B8B+1083] (c:\src\git-chrome\src\components\discardable_memory\service\discardable_shared_memory_manager.cc:242)
        discardable_memory::`anonymous namespace'::TestDiscardableSharedMemoryManager::TestDiscardableSharedMemoryManager [0x0000000143322024+20] (c:\src\git-chrome\src\components\discardable_memory\service\discardable_shared_memory_manager_unittest.cc:39)
        discardable_memory::`anonymous namespace'::DiscardableSharedMemoryManagerTest::SetUp [0x0000000143322F0A+42] (c:\src\git-chrome\src\components\discardable_memory\service\discardable_shared_memory_manager_unittest.cc:65)
        testing::internal::HandleExceptionsInMethodIfSupported<testing::Test,void> [0x0000000144027068+72] (c:\src\git-chrome\src\third_party\googletest\src\googletest\src\gtest.cc:2457)
        testing::Test::Run [0x000000014404AA4B+91] (c:\src\git-chrome\src\third_party\googletest\src\googletest\src\gtest.cc:2469)
        testing::TestInfo::Run [0x000000014404AD81+209] (c:\src\git-chrome\src\third_party\googletest\src\googletest\src\gtest.cc:2657)
        testing::TestCase::Run [0x000000014404ABE1+225] (c:\src\git-chrome\src\third_party\googletest\src\googletest\src\gtest.cc:2772)
        testing::internal::UnitTestImpl::RunAllTests [0x000000014404B2ED+813] (c:\src\git-chrome\src\third_party\googletest\src\googletest\src\gtest.cc:4649)
        testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,bool> [0x0000000144027188+72] (c:\src\git-chrome\src\third_party\googletest\src\googletest\src\gtest.cc:2457)
        testing::UnitTest::Run [0x000000014404AF3C+236] (c:\src\git-chrome\src\third_party\googletest\src\googletest\src\gtest.cc:4256)
        RUN_ALL_TESTS [0x0000000146A0B211+17] (c:\src\git-chrome\src\third_party\googletest\src\googletest\include\gtest\gtest.h:2238)
        base::TestSuite::Run [0x0000000146A0B46C+140] (c:\src\git-chrome\src\base\test\test_suite.cc:270)
        content::UnitTestTestSuite::Run [0x0000000147DE5AFE+30] (c:\src\git-chrome\src\content\public\test\unittest_test_suite.cc:46)
        base::internal::FunctorTraits<int (__cdecl content::UnitTestTestSuite::*)(void) __ptr64,void>::Invoke<std::unique_ptr<content::UnitTestTestSuite,std::default_delete<content::UnitTestTestSuite> > const & __ptr64> [0x00000001443EFB6F+31] (c:\src\git-chrome\src\base\bind_internal.h:210)
        base::internal::InvokeHelper<0,int>::MakeItSo<int (__cdecl content::UnitTestTestSuite::*const & __ptr64)(void) __ptr64,std::unique_ptr<content::UnitTestTestSuite,std::default_delete<content::UnitTestTestSuite> > const & __ptr64> [0x00000001443EFBF7+55] (c:\src\git-chrome\src\base\bind_internal.h:277)
        base::internal::Invoker<base::internal::BindState<int (__cdecl content::UnitTestTestSuite::*)(void) __ptr64,std::unique_ptr<content::UnitTestTestSuite,std::default_delete<content::UnitTestTestSuite> > >,int __cdecl(void)>::RunImpl<int (__cdecl content::Un [0x00000001443EFCDC+76] (c:\src\git-chrome\src\base\bind_internal.h:355)
        base::internal::Invoker<base::internal::BindState<int (__cdecl content::UnitTestTestSuite::*)(void) __ptr64,std::unique_ptr<content::UnitTestTestSuite,std::default_delete<content::UnitTestTestSuite> > >,int __cdecl(void)>::Run [0x00000001443F09F3+51] (c:\src\git-chrome\src\base\bind_internal.h:333)
        base::Callback<int __cdecl(void),1,1>::Run [0x0000000146A0047A+42] (c:\src\git-chrome\src\base\callback.h:81)
        base::`anonymous namespace'::LaunchUnitTestsInternal [0x00000001469FF489+329] (c:\src\git-chrome\src\base\test\launcher\unit_test_launcher.cc:216)
        base::LaunchUnitTests [0x00000001469FF219+137] (c:\src\git-chrome\src\base\test\launcher\unit_test_launcher.cc:462)
        main [0x0000000142E28791+49] (c:\src\git-chrome\src\components\test\run_all_unittests.cc:8)
        invoke_main [0x0000000149EC9D44+52] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:65)
        __scrt_common_main_seh [0x0000000149EC9C07+295] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:253)
        __scrt_common_main [0x0000000149EC9ACE+14] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:296)
        mainCRTStartup [0x0000000149EC9D69+9] (f:\dd\vctools\crt\vcstartup\src\startup\exe_main.cpp:17)
        BaseThreadInitThunk [0x00007FFB002A8102+34]
        RtlUserThreadStart [0x00007FFB01F3C5B4+52]

 

Comment 1 by w...@chromium.org, Jul 22 2017

Cc: penny...@chromium.org haraken@chromium.org
Labels: -Pri-3 M-62 Pri-2
+haraken for MemoryCoordinator, +pennymac re VirtualFree(MEM_DECOMMIT).

Comment 2 by w...@chromium.org, Aug 13 2017

Owner: w...@chromium.org
Status: Assigned (was: Untriaged)

Comment 3 by w...@chromium.org, Aug 15 2017

Status: Started (was: Assigned)
Working with erikchen@, I've confirmed that VirtualFree(MEM_DECOMMIT) always fails in Purgae() (see the patch at https://chromium-review.googlesource.com/c/582031 to repro the log output we used).

I ran some simple tests which:

1. Create a 1GB shared-memory instance.
2. Repeatedly (30 times in my test):
  a. Lock all the pages.
  b. Write to all the pages with memset().
  c. Unlock all the pages.
  d. Purge() all the pages.

gives a slow-down of 6x if we introduce Offer/Reclaim in the UnlockLock calls.

Introducing DiscardVirtualMemory() has no discernible impact on the speed of the 30 calls, but does cause the pages to be correctly discarded, so I suggest we just replace MEM_DECOMMIT with that in the Purge() impl.

Project Member

Comment 4 by bugdroid1@chromium.org, Aug 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/17da1b43d88c350c551f61b8f8b87dae14c62306

commit 17da1b43d88c350c551f61b8f8b87dae14c62306
Author: Wez <wez@chromium.org>
Date: Fri Aug 18 21:28:54 2017

Replace VirtualFree(DECOMMIT) with DiscardVirtualMemory in Purge().

Windows does not allow page-file backed memory-mapped file pages to be
de-committed, so the call to VirtualFree(MEM_DECOMMIT) always fails.

Windows 8.1 and above provide DiscardVirtualMemory(), which releases
the underlying storage for pages without decommitting them. This is
equivalent to swapping-out the pages, freeing up the physical memory
they occupied, but without any actual paging activity, since their
contents are being lost. The pages continue to be accounted against
the system's total commit charge, so this won't prevent apps OOMing
if the available commit charge is low.

Bug:  747657 
Change-Id: Icf6d4b16c1adcc545b09bad207a9abd4a97e4726
Reviewed-on: https://chromium-review.googlesource.com/615062
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Penny MacNeil <pennymac@chromium.org>
Commit-Queue: Penny MacNeil <pennymac@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495691}
[modify] https://crrev.com/17da1b43d88c350c551f61b8f8b87dae14c62306/base/memory/discardable_shared_memory.cc

Comment 5 by w...@chromium.org, Aug 22 2017

Status: Fixed (was: Started)
We've removed the failing API call, and I'm unable to repro the ObserverList failure any more, so closing this out.

Comment 6 by w...@chromium.org, Nov 27 2017

 Issue 762674  has been merged into this issue.

Sign in to add a comment