Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions tiledb/common/logger.cc
Original file line number Diff line number Diff line change
Expand Up @@ -329,8 +329,12 @@ std::string global_logger_name(const Logger::Format format) {
}

Logger& global_logger(Logger::Format format) {
static Logger l(global_logger_name(format), Logger::Level::ERR, format, true);
return l;
// Note: We are *not* deallocating this root `Logger` instance so that
// during process exit, threads other than main will not trigger segfault's
// if they attempt to log after the main thread has exited.
static Logger* l = tiledb_new<Logger>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the heap profiler ever enabled by the time you get here? It looks like it is enabled only via C API, not even via environmental option on startup

I suppose if that changes then you will pick it up here which will be nice - unless it expects memory usage to be zero by the time it exits for some reason

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To my knowledge the heap profiling is a compile time option so “yes” I guess? Assuming that code still works.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have expected it to be something that has to last for the life of the process but there is a C API tiledb_heap_profiler_enable which looks like it connects to the same thing used here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick skim suggests that the reporter logic is always available, but if its not enabled at compile time it’ll just not have anything to report.

See:

#if defined(TILEDB_MEMTRACE)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I'm looking at is:

In tiledb/common/heap_memory.h:

template <typename T, typename... Args>
T* tiledb_new(const std::string& label, Args&&... args) {
  if (!heap_profiler.enabled()) {
    return new T(std::forward<Args>(args)...);
  }
  ...
}

In tiledb/common/heap_profiler.h:

extern HeapProfiler heap_profiler;

class HeapProfiler {
  inline bool enabled() const {
    // We know that this instance has been initalized
    // if `reserved_memory_` has been set.
    return reserved_memory_ != nullptr;
  }
};

And in tiledb/common/heap_profiler.cc we have HeapProfiler::HeapProfiler initializing its reserved_memory_ to null. The enable function I mentioned above initializes this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. Looks like that’s a missed optimization issue. The make_shared thing was updated specifically to be compile time switch. You can see the constexpr part here:

if constexpr (detail::global_tracing<void>::enabled::value) {

HERE(), global_logger_name(format), Logger::Level::ERR, format, true);
return *l;
}

/** Logs a trace. */
Expand Down
4 changes: 2 additions & 2 deletions tiledb/sm/global_state/global_state.cc
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,9 @@ using namespace tiledb::common;

namespace tiledb::sm::global_state {

GlobalState& GlobalState::GetGlobalState() {
shared_ptr<GlobalState> GlobalState::GetGlobalState() {
// This is thread-safe in C++11.
static GlobalState globalState;
static shared_ptr<GlobalState> globalState = make_shared<GlobalState>(HERE());
return globalState;
}

Expand Down
9 changes: 5 additions & 4 deletions tiledb/sm/global_state/global_state.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
#include <set>
#include <string>

#include "tiledb/common/common.h"
#include "tiledb/sm/config/config.h"
#include "tiledb/sm/storage_manager/storage_manager_declaration.h"

Expand All @@ -59,8 +60,11 @@ class GlobalState {
GlobalState& operator=(const GlobalState&) = delete;
GlobalState& operator=(const GlobalState&&) = delete;

/** Constructor. */
GlobalState();

/** Returns a reference to the singleton GlobalState instance. */
static GlobalState& GetGlobalState();
static shared_ptr<GlobalState> GetGlobalState();

/**
* Initializes all TileDB global state in an idempotent and threadsafe way.
Expand Down Expand Up @@ -101,9 +105,6 @@ class GlobalState {

/** Mutex protecting list of StorageManagers. */
std::mutex storage_managers_mtx_;

/** Constructor. */
GlobalState();
};

} // namespace global_state
Expand Down
4 changes: 3 additions & 1 deletion tiledb/sm/global_state/watchdog.cc
Original file line number Diff line number Diff line change
Expand Up @@ -73,13 +73,15 @@ void Watchdog::watchdog_thread(Watchdog* watchdog) {
return;
}

auto global_state = GlobalState::GetGlobalState();

while (true) {
std::unique_lock<std::mutex> lck(watchdog->mtx_);
watchdog->cv_.wait_for(
lck, std::chrono::milliseconds(constants::watchdog_thread_sleep_ms));

if (SignalHandlers::signal_received()) {
for (auto* sm : GlobalState::GetGlobalState().storage_managers()) {
for (auto* sm : global_state->storage_managers()) {
throw_if_not_ok(sm->cancel_all_tasks());
}
}
Expand Down
2 changes: 2 additions & 0 deletions tiledb/sm/global_state/watchdog.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@
#include <mutex>
#include <thread>

#include "tiledb/sm/global_state/global_state.h"

namespace tiledb {
namespace sm {
namespace global_state {
Expand Down
2 changes: 2 additions & 0 deletions tiledb/sm/storage_manager/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,5 @@ commence(object_library context_resources)
this_target_sources(context_resources.cc)
this_target_object_libraries(baseline config rest_client stats thread_pool vfs)
conclude(object_library)

add_test_subdirectory()
11 changes: 5 additions & 6 deletions tiledb/sm/storage_manager/storage_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -60,18 +60,17 @@ StorageManagerCanonical::StorageManagerCanonical(
ContextResources& resources,
const shared_ptr<Logger>&, // unused
const Config& config)
: vfs_(resources.vfs())
: global_state_(global_state::GlobalState::GetGlobalState())
, vfs_(resources.vfs())
, cancellation_in_progress_(false)
, config_(config)
, queries_in_progress_(0) {
auto& global_state = global_state::GlobalState::GetGlobalState();
global_state.init(config_);

global_state.register_storage_manager(this);
global_state_->init(config_);
global_state_->register_storage_manager(this);
}

StorageManagerCanonical::~StorageManagerCanonical() {
global_state::GlobalState::GetGlobalState().unregister_storage_manager(this);
global_state_->unregister_storage_manager(this);

throw_if_not_ok(cancel_all_tasks());

Expand Down
4 changes: 4 additions & 0 deletions tiledb/sm/storage_manager/storage_manager_canonical.h
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
#include "tiledb/sm/array/array_directory.h"
#include "tiledb/sm/enums/walk_order.h"
#include "tiledb/sm/filesystem/uri.h"
#include "tiledb/sm/global_state/global_state.h"
#include "tiledb/sm/group/group.h"
#include "tiledb/sm/misc/cancelable_tasks.h"
#include "tiledb/sm/misc/types.h"
Expand Down Expand Up @@ -146,6 +147,9 @@ class StorageManagerCanonical {
/* PRIVATE ATTRIBUTES */
/* ********************************* */

/** The GlobalState to use for this StorageManager. */
shared_ptr<global_state::GlobalState> global_state_;

/**
* VFS instance used in `cancel_all_tasks`.
*/
Expand Down
35 changes: 35 additions & 0 deletions tiledb/sm/storage_manager/test/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
#
# tiledb/sm/storage_manager/CMakeLists.txt
#
# The MIT License
#
# Copyright (c) 2025 TileDB, Inc.
#
# Permission is hereby granted, free of charge, to any person obtaining a copy
# of this software and associated documentation files (the "Software"), to deal
# in the Software without restriction, including without limitation the rights
# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
# copies of the Software, and to permit persons to whom the Software is
# furnished to do so, subject to the following conditions:
#
# The above copyright notice and this permission notice shall be included in
# all copies or substantial portions of the Software.
#
# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
# THE SOFTWARE.
#

include(unit_test)

commence(unit_test static_context)
this_target_sources(main.cc unit_static_context.cc)
target_include_directories(unit_static_context PRIVATE
"$<TARGET_PROPERTY:TILEDB_CORE_OBJECTS,INCLUDE_DIRECTORIES>")
this_target_link_libraries(TILEDB_CORE_OBJECTS)
this_target_link_libraries(tiledb_test_support_lib)
conclude(unit_test)
34 changes: 34 additions & 0 deletions tiledb/sm/storage_manager/test/main.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/**
* @file tiledb/sm/storage_manager/test/main.cc
*
* @section LICENSE
*
* The MIT License
*
* @copyright Copyright (c) 2021 TileDB, Inc.
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*
* @section DESCRIPTION
*
* This file defines a test `main()`
*/

#define CATCH_CONFIG_MAIN
#include <test/support/tdb_catch.h>
65 changes: 65 additions & 0 deletions tiledb/sm/storage_manager/test/unit_static_context.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/**
* @file tiledb/sm/storage_manager/test/unit_static_context.cc
*
* @section LICENSE
*
* The MIT License
*
* @copyright Copyright (c) 2021-2025 TileDB, Inc.
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*
* @section DESCRIPTION
*
* This file defines unit tests for the checking StorageManager and GlobalState
* lifetimes.
*/

#include <test/support/tdb_catch.h>
#if defined(DELETE)
#undef DELETE
#endif
#include <tiledb/sm/array/array.h>
#include <tiledb/sm/array_schema/dimension.h>
#include <tiledb/sm/enums/array_type.h>
#include <tiledb/sm/enums/encryption_type.h>
#include <tiledb/sm/storage_manager/context.h>
#include <tiledb/sm/subarray/subarray.h>

#include <test/support/src/helpers.h>
#include <test/support/src/mem_helpers.h>

using namespace tiledb;
using namespace tiledb::common;
using namespace tiledb::sm;
using namespace tiledb::type;

TEST_CASE("Static Context", "[context]") {
// While non-obvious from this implementation, the issue here is that the
// destructor of the `static std::optional<Context>` is run after the other
// static instance finalizers for GlobalState and Logger. This ordering ends
// up causing a segfault when the `Context` attempts to use those resources
// after they have been destructed.
//
// Thus, the actual assertion of this test is that we don't segfault which
// is reported as an error by Catch2.
static tiledb::Config cfg;
static std::optional<tiledb::Context> ctx;
ctx = std::make_optional(tiledb::Context(cfg));
}
Loading