Repository navigation
chore(umbp): remove dead code, an orphaned test and duplicated helpers - #778
Merged
Merged
Conversation
Everything removed here has no caller anywhere in the repo (C++, pybind, Python, tests); most of it lost its last caller in the backend-agnostic refactor (#540). No behavior change. Dead files - local/tiers/device_copy_run.h: run planner for the removed DRAMTier. - local/tiers/{device_gather,host_registration}.h: "backward-compatible" shims that nothing includes. - tests/cpp/umbp/local/test_spdk_proxy.cpp: never in any CMakeLists, so it has never been built. Dead functions and state - PoolClient::ExecuteLocalGet (+ GetAttemptOutcome), CopyRangesToContiguous. - SpdkSsdTier::Batch{Write,Read}Dma{Direct,Streaming} (~490 lines). - TierBackend::ReadPtr / BatchWriteRanges / GetLocationId / SetColdRead and their SSDTier / ShardedSsdTier / SpdkProxyTier overrides; SpdkProxyTier's cold_read_ was therefore always false. - SSDTier::last_io_status_: written at 13 sites (racily, from const reads), never read. - SpdkEnv reactor accessors, PollIo, CleanupThreadLocalCtx, CleanupOnSpdkThread, bdev_; ProxyShmRegion::ProbeExisting and its HeartbeatStaleMs; SpdkProxyTier::WaitForProxy and ProxyPollInterval; TenantRegistry::FindTenant. - Unused accessors/constants: HostTierRegistration::Covers/RegisteredBytes, MasterClient::SupportsLogicalTiers, MoriIoEngine::LocalEngineKey, UMBPConfig::ResolveRole, kAutoRankId, kLocFieldMarker, LocalLocation, SegmentIndex::ClearKnownSegments, ThreadDoesBackgroundCopies, two legacy SPDK proxy aliases, write-only PeerSsdManager eviction counters and io_uring ring-entry fields. - route_get_strategy.cpp: commented-out debug logging and the SummarizeLocations helper only it used. Duplicated code - Five identical TierType <-> proto switch converters (three standalone files, peer_service.cpp, batch_resolve_codec.h) are now one inline pair in umbp/distributed/tier_proto.h. - Three identical MatchedHashCount bodies now share CountDistinctHashes(). The two Python-visible match types stay distinct classes. Also drops test_prefix_aware_eviction from the CI ctest regex: no such test has existed since #540. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Removes UMBP code that nothing calls, a test that has never been built, and five copies of the same helper. Most of it lost its last caller in the backend-agnostic refactor (#540). No behavior change. Net −1,370 lines across 45 files.
Each removal was checked three ways:
objdump -rscan over every object file of a full UMBP build, to catch dead functions whose names are too common for grep.GetLocationId,SetColdRead).Dead files
local/tiers/device_copy_run.hlocal/tiers/device_gather.h,local/tiers/host_registration.hinclude/umbp/common/)tests/cpp/umbp/local/test_spdk_proxy.cppCMakeLists.txt, at any point in history, so it has never been compiledDead functions and state
PoolClient::ExecuteLocalGet(andGetAttemptOutcome) andPoolClient::CopyRangesToContiguous.SpdkSsdTier::Batch{Write,Read}Dma{Direct,Streaming}, about 490 lines.TierBackend::ReadPtr,BatchWriteRanges,GetLocationId,SetColdRead, plus theirSSDTier,ShardedSsdTierandSpdkProxyTieroverrides. Because nothing ever set it,SpdkProxyTier::cold_read_was always false; the field and its branch are removed.SSDTier::last_io_status_(withLastIoStatus/RememberStatus): written at 13 sites but never read. The writes ran concurrently fromconstread paths, so this also removes a latent data race.SpdkEnv: the reactor accessors,PollIo,CleanupThreadLocalCtx,CleanupOnSpdkThreadandbdev_.ProxyShmRegion::ProbeExistingand its file-localHeartbeatStaleMs.SpdkProxyTier::WaitForProxyandProxyPollInterval.UMBP_SPDK_PROXY_POLL_INTERVAL_MSis dropped fromruntime-env-vars.mdbecause nothing reads it now.TenantRegistry::FindTenant.HostTierRegistration::CoversandRegisteredBytes.MasterClient::SupportsLogicalTiers,MoriIoEngine::LocalEngineKey,UMBPConfig::ResolveRole,kAutoRankId,kLocFieldMarker,LocalLocation,SegmentIndex::ClearKnownSegments,ThreadDoesBackgroundCopies.PeerSsdManagereviction counters (eviction is counted by the decorator and the pool) and write-only io_uring ring-entry fields.route_get_strategy.cpp: commented-out debug logging, plusSummarizeLocations, which only that logging used.Duplicated code
TierType↔::umbp::TierTypeswitch converters (threestandalone/*.cppfiles,peer_service.cpp,batch_resolve_codec.h) are now one inline pair inumbp/distributed/tier_proto.h. Thestatic_castversions inmaster_client.cppbehave differently on unknown values, so they are left alone.MatchedHashCount()bodies now call one sharedCountDistinctHashes()intypes.h.IUMBPClient::ExternalKvMatchandMasterClient::ExternalKvNodeMatchstay separate types because pybind registers them as two different Python classes.Also
.github/workflows/ci.yml: removestest_prefix_aware_evictionfrom the UMBP ctest regex. No test by that name has existed since refactor(umbp): make distributed mode backend- and transport-agnostic #540.Kept on purpose
TierBackend::ReadBatchRangesIntoPtrand theranged_readcapability: only tests call them today, but they are the building block for the still-open D1 phase ofdesign-ssd-ranged-io.md.IMasterMetadataStore::LookupBlock,GetExternalKvCount, ...), the vendored SPDKoffset_allocator, and the bench and run scripts (all referenced).bench_pool_client_batch_{get,put}.cpp, or whose env parsing differs slightly. Merging those would be a refactor, not a deletion.Test plan
Before/after comparison on n06-21 (
local/mori-dev:sglang-0.5.19-*, gfx950). Build flags:BUILD_UMBP=ON,USE_REDIS_BACKEND=ON,BUILD_TESTS=ON, pybindmori_umbp_pybinds.mainand this branch. Compiler warnings are identical (same 100).tests/cpp/umbp, 57 ctests): per-test outcomes are identical betweenmainand this branch, 46 pass. The other 11 are PoolClient/IO-engine tests that need a GPU; they fail identically on both withrsmi_num_monitor_devices reported 0 GPUs, because the container had no GPU while the node's GPUs were fully busy with another job. CI runs them with a GPU.pytest tests/python/umbp/): 105/105 on both, loading the freshly builtlibmori_umbp_pybinds.so(md5 verified).matched_hash_count()returns the same value on both match types.spdk_env.cpp,spdk_ssd_tier.cpp,spdk_proxy_daemon.cpp): not compiled, because SPDK isn't available in the build image. Every edit there is a pure deletion of unreferenced code (braces balanced, no remaining references), but it still needs an SPDK build to confirm.hipfile): not compiled, but untouched.🤖 Generated with Claude Code