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

Issue 590907 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 448794
issue 588824



Sign in to add a comment

gpu tests Run Out of TLS Slots When Using ThreadLocalStorage

Project Member Reported by robliao@chromium.org, Feb 29 2016

Issue description

tools/valgrind/chrome_tests.sh --test gpu --tool memcheck --target Release

[----------] 192 tests from Service/FeatureInfoTest
[ RUN      ] Service/FeatureInfoTest.Basic/0
[       OK ] Service/FeatureInfoTest.Basic/0 (113 ms)
[ RUN      ] Service/FeatureInfoTest.Basic/1
[       OK ] Service/FeatureInfoTest.Basic/1 (41 ms)
[ RUN      ] Service/FeatureInfoTest.InitializeNoExtensions/0
[       OK ] Service/FeatureInfoTest.InitializeNoExtensions/0 (177 ms)
[ RUN      ] Service/FeatureInfoTest.InitializeNoExtensions/1
[       OK ] Service/FeatureInfoTest.InitializeNoExtensions/1 (138 ms)
[ RUN      ] Service/FeatureInfoTest.InitializeWithANGLE/0
[       OK ] Service/FeatureInfoTest.InitializeWithANGLE/0 (55 ms)
[ RUN      ] Service/FeatureInfoTest.InitializeWithANGLE/1
[12611:12611:0225/202257:606318529:FATAL:thread_local_storage.cc(213)] Check failed: slot_ < kThreadLocalStorageSize (256 vs. 256)
#0 0x000000928d53 base::debug::StackTrace::StackTrace()
#1 0x0000008cb472 logging::LogMessage::~LogMessage()
#2 0x000000907159 base::ThreadLocalStorage::StaticSlot::Initialize()
#3 0x000000907056 base::ThreadLocalStorage::Slot::Slot()
#4 0x0000009bf09b base::ThreadLocalPointer<>::ThreadLocalPointer()
#5 0x0000009befbb gfx::InitializeStaticGLBindingsGL()
#6 0x0000009c16a3 gfx::InitializeStaticGLBindings()
#7 0x0000009c35a9 gfx::GLSurface::InitializeOneOffImplementation()
#8 0x0000009dac67 gfx::GLSurfaceTestSupport::InitializeOneOffImplementation()
#9 0x0000009dacc0 gfx::GLSurfaceTestSupport::InitializeOneOffWithMockBindings()
#10 0x0000007bd6ed gpu::gles2::GpuServiceTest::SetUpWithGLVersion()
#11 0x00000059c632 gpu::gles2::FeatureInfoTest::SetupInitExpectationsWithGLVersion()
#12 0x00000059c546 gpu::gles2::FeatureInfoTest_InitializeWithANGLE_Test::TestBody()
#13 0x000000992467 testing::internal::HandleSehExceptionsInMethodIfSupported<>()
#14 0x000000984e09 testing::internal::HandleExceptionsInMethodIfSupported<>()
#15 0x000000984d96 testing::Test::Run()
#16 0x0000009854d3 testing::TestInfo::Run()
#17 0x000000985b52 testing::TestCase::Run()
#18 0x000000989f53 testing::internal::UnitTestImpl::RunAllTests()
#19 0x000000993df7 testing::internal::HandleSehExceptionsInMethodIfSupported<>()
#20 0x000000989d39 testing::internal::HandleExceptionsInMethodIfSupported<>()
#21 0x000000989c83 testing::UnitTest::Run()
#22 0x0000009600c1 RUN_ALL_TESTS()
#23 0x00000095fbea base::TestSuite::Run()
#24 0x0000005765bc _ZN4base8internal15RunnableAdapterIMNS_9TestSuiteEFivEE3RunIJEEEiPS2_DpOT_
#25 0x000000576564 _ZN4base8internal12InvokeHelperILb0EiNS0_15RunnableAdapterIMNS_9TestSuiteEFivEEEE8MakeItSoIJPS3_EEEiS6_DpOT_
#26 0x000000576537 _ZN4base8internal7InvokerINS_13IndexSequenceIJLm0EEEENS0_9BindStateINS0_15RunnableAdapterIMNS_9TestSuiteEFivEEEFiPS6_EJNS0_17UnretainedWrapperIS6_EEEEENS0_12InvokeHelperILb0EiS9_EEFivEE3RunEPNS0_13BindStateBaseE
#27 0x00000095bbb4 base::Callback<>::Run()
#28 0x00000095aeb1 base::(anonymous namespace)::LaunchUnitTestsInternal()
#29 0x00000095ad8d base::LaunchUnitTests()
#30 0x0000005762c7 main
#31 0x00000699a76d __libc_start_main
#32 0x000000450e85 <unknown>
 
Blocking: -588824
Blocking: 588824
Cc: halliwell@chromium.org

Comment 4 by sanfin@chromium.org, Apr 26 2016

Cc: sanfin@chromium.org
Context:
The tests run out of TLS slots because ThreadLocalStorage doesn't actually implement free:

void ThreadLocalStorage::StaticSlot::Free() {
  // At this time, we don't reclaim old indices for TLS slots.
  // So all we need to do is wipe the destructor.

Since TLS slots are global to a process, tests that utilize the TLS could run out if enough of them run in a single process/shard.

One solution here would be to have a vector track the alloced/dealloced state.
Cc: siev...@chromium.org jbudorick@chromium.org kbr@chromium.org klundberg@chromium.org nduca@chromium.org epenner@chromium.org
 Issue 460257  has been merged into this issue.
Blocking: 448794
Project Member

Comment 8 by bugdroid1@chromium.org, Sep 28 2016

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

commit 783d1f9630c86d816669ab23981d0f3722c96808
Author: robliao <robliao@chromium.org>
Date: Tue Sep 27 20:27:43 2016

Add Reclaim Support to ThreadLocalStorage

Previously, ThreadLocalStorage::StaticSlot::Free() did not actually
release the corresponding ThreadLocalStorage slot. It simply cleared
out the slot and did not reuse it. As a result, each process had a
finite number of calls to ThreadLocalStorage::StaticSlot::Initialize()
before running out of slots.

This problem would manifest itself in tests where a single process runs
many tests that each do their own initialization and uninitialization.
Tests that involve TLS usage caused the process to run out of TLS slots
because there was no free support in ThreadLocalStorage.

This change adds in free support by doing what most operating systems
do, lock and track metadata in an array.

BUG= 590907 

Review-Url: https://codereview.chromium.org/2363783002
Cr-Commit-Position: refs/heads/master@{#421320}

[modify] https://crrev.com/783d1f9630c86d816669ab23981d0f3722c96808/base/threading/thread_local_storage.cc
[modify] https://crrev.com/783d1f9630c86d816669ab23981d0f3722c96808/base/threading/thread_local_storage_unittest.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Oct 6 2016

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

commit 75f2f7f6f9bdf3d851b5cc3914d32a3d0836c49d
Author: robliao <robliao@chromium.org>
Date: Thu Oct 06 17:37:31 2016

Revert of Add Reclaim Support to ThreadLocalStorage (patchset #1 id:1 of https://codereview.chromium.org/2363783002/ )

Reason for revert:
Precautionary revert while waiting for https://codereview.chromium.org/2383833004/

Will reland after branch point.

Original issue's description:
> Add Reclaim Support to ThreadLocalStorage
>
> Previously, ThreadLocalStorage::StaticSlot::Free() did not actually
> release the corresponding ThreadLocalStorage slot. It simply cleared
> out the slot and did not reuse it. As a result, each process had a
> finite number of calls to ThreadLocalStorage::StaticSlot::Initialize()
> before running out of slots.
>
> This problem would manifest itself in tests where a single process runs
> many tests that each do their own initialization and uninitialization.
> Tests that involve TLS usage caused the process to run out of TLS slots
> because there was no free support in ThreadLocalStorage.
>
> This change adds in free support by doing what most operating systems
> do, lock and track metadata in an array.
>
> BUG= 590907 
>
> Committed: https://crrev.com/783d1f9630c86d816669ab23981d0f3722c96808
> Cr-Commit-Position: refs/heads/master@{#421320}

TBR=brettw@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 590907 

Review-Url: https://codereview.chromium.org/2395043002
Cr-Commit-Position: refs/heads/master@{#423575}

[modify] https://crrev.com/75f2f7f6f9bdf3d851b5cc3914d32a3d0836c49d/base/threading/thread_local_storage.cc
[modify] https://crrev.com/75f2f7f6f9bdf3d851b5cc3914d32a3d0836c49d/base/threading/thread_local_storage_unittest.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Oct 6 2016

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

commit 745d6044a0560f4470e005f1a92543f889600323
Author: robliao <robliao@chromium.org>
Date: Thu Oct 06 22:20:54 2016

Scale Stack Limits in ThreadTest.StartWithOptions_StackSize with Bitness

12 kb may be too small for a 64-bit machine. This allows enough
breathing space for the thread local storage teardown later on.

BUG= 590907 

Review-Url: https://codereview.chromium.org/2395303002
Cr-Commit-Position: refs/heads/master@{#423710}

[modify] https://crrev.com/745d6044a0560f4470e005f1a92543f889600323/base/threading/thread_unittest.cc

Project Member

Comment 11 by bugdroid1@chromium.org, Oct 7 2016

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

commit 45609b5403d73864e5119cd5084bd6f9c7f62076
Author: robliao <robliao@chromium.org>
Date: Fri Oct 07 18:09:20 2016

Add Reclaim Support to ThreadLocalStorage

Previously, ThreadLocalStorage::StaticSlot::Free() did not actually
release the corresponding ThreadLocalStorage slot. It simply cleared
out the slot and did not reuse it. As a result, each process had a
finite number of calls to ThreadLocalStorage::StaticSlot::Initialize()
before running out of slots.

This problem would manifest itself in tests where a single process runs
many tests that each do their own initialization and uninitialization.
Tests that involve TLS usage caused the process to run out of TLS slots
because there was no free support in ThreadLocalStorage.

This change adds in free support by doing what most operating systems
do, lock and track metadata in an array.

BUG= 590907 

Committed: https://crrev.com/783d1f9630c86d816669ab23981d0f3722c96808
Review-Url: https://codereview.chromium.org/2363783002
Cr-Original-Commit-Position: refs/heads/master@{#421320}
Cr-Commit-Position: refs/heads/master@{#423911}

[modify] https://crrev.com/45609b5403d73864e5119cd5084bd6f9c7f62076/base/threading/thread_local_storage.cc
[modify] https://crrev.com/45609b5403d73864e5119cd5084bd6f9c7f62076/base/threading/thread_local_storage_unittest.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Oct 7 2016

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

commit c90ffe2a6c10bb34c8eed5225f6d3e4d3be23acb
Author: robliao <robliao@chromium.org>
Date: Fri Oct 07 20:22:12 2016

Ensure Freed TLS Slots Contain nullptr on Reallocation

Code generally depends on TLS slots initialized to zero. This means
that code that gets a reused slot also depends on the reused slot
containing zero.

This CL introduces a versioning system to allow us to quickly determine
if a slot was previously freed.

Also added overview comments as a bonus.

BUG= 590907 

Review-Url: https://codereview.chromium.org/2383833004
Cr-Commit-Position: refs/heads/master@{#423952}

[modify] https://crrev.com/c90ffe2a6c10bb34c8eed5225f6d3e4d3be23acb/base/threading/thread_local_storage.cc
[modify] https://crrev.com/c90ffe2a6c10bb34c8eed5225f6d3e4d3be23acb/base/threading/thread_local_storage.h
[modify] https://crrev.com/c90ffe2a6c10bb34c8eed5225f6d3e4d3be23acb/base/threading/thread_local_storage_unittest.cc

Status: Fixed (was: Assigned)
Project Member

Comment 14 by bugdroid1@chromium.org, Oct 27 2016

Labels: merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/75f2f7f6f9bdf3d851b5cc3914d32a3d0836c49d

commit 75f2f7f6f9bdf3d851b5cc3914d32a3d0836c49d
Author: robliao <robliao@chromium.org>
Date: Thu Oct 06 17:37:31 2016

Revert of Add Reclaim Support to ThreadLocalStorage (patchset #1 id:1 of https://codereview.chromium.org/2363783002/ )

Reason for revert:
Precautionary revert while waiting for https://codereview.chromium.org/2383833004/

Will reland after branch point.

Original issue's description:
> Add Reclaim Support to ThreadLocalStorage
>
> Previously, ThreadLocalStorage::StaticSlot::Free() did not actually
> release the corresponding ThreadLocalStorage slot. It simply cleared
> out the slot and did not reuse it. As a result, each process had a
> finite number of calls to ThreadLocalStorage::StaticSlot::Initialize()
> before running out of slots.
>
> This problem would manifest itself in tests where a single process runs
> many tests that each do their own initialization and uninitialization.
> Tests that involve TLS usage caused the process to run out of TLS slots
> because there was no free support in ThreadLocalStorage.
>
> This change adds in free support by doing what most operating systems
> do, lock and track metadata in an array.
>
> BUG= 590907 
>
> Committed: https://crrev.com/783d1f9630c86d816669ab23981d0f3722c96808
> Cr-Commit-Position: refs/heads/master@{#421320}

TBR=brettw@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 590907 

Review-Url: https://codereview.chromium.org/2395043002
Cr-Commit-Position: refs/heads/master@{#423575}

[modify] https://crrev.com/75f2f7f6f9bdf3d851b5cc3914d32a3d0836c49d/base/threading/thread_local_storage.cc
[modify] https://crrev.com/75f2f7f6f9bdf3d851b5cc3914d32a3d0836c49d/base/threading/thread_local_storage_unittest.cc

Project Member

Comment 15 by bugdroid1@chromium.org, Oct 27 2016

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

commit 745d6044a0560f4470e005f1a92543f889600323
Author: robliao <robliao@chromium.org>
Date: Thu Oct 06 22:20:54 2016

Scale Stack Limits in ThreadTest.StartWithOptions_StackSize with Bitness

12 kb may be too small for a 64-bit machine. This allows enough
breathing space for the thread local storage teardown later on.

BUG= 590907 

Review-Url: https://codereview.chromium.org/2395303002
Cr-Commit-Position: refs/heads/master@{#423710}

[modify] https://crrev.com/745d6044a0560f4470e005f1a92543f889600323/base/threading/thread_unittest.cc

Comment 16 by dimu@google.com, Nov 4 2016

Labels: -merge-merged-2840
[Automated comment] removing mislabelled merge-merged-2840

Sign in to add a comment