Devirtualization + internal linkage: mark leaf impl classes final and TU-local helpers static#1488
Open
bmehta001 wants to merge 2 commits into
Open
Devirtualization + internal linkage: mark leaf impl classes final and TU-local helpers static#1488bmehta001 wants to merge 2 commits into
bmehta001 wants to merge 2 commits into
Conversation
Mark the leaf concrete implementations of IHttpClient and IOfflineStorage final
so the compiler can devirtualize (and often inline) calls made through them:
HttpClient_{Curl,WinInet,WinRt,Apple,Android,CAPI} and OfflineStorage_Room /
MemoryStorage / OfflineStorageHandler.
Each was verified to have no subclass anywhere in the tree (lib + tests +
wrappers). Deliberately NOT marked:
- OfflineStorage_SQLite -- tests/unittests/OfflineStorageTests_SQLite.cpp
subclasses it (OfflineStorage_SQLiteNoAutoCommit).
- TelemetrySystemBase -- base of TelemetrySystem / AITelemetrySystem.
Validated: NDK aarch64 -fsyntax-only on OfflineStorage_Room, OfflineStorageHandler
and MemoryStorage.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR optimizes hot-path virtual dispatch by marking leaf concrete implementations of the IHttpClient and IOfflineStorage interfaces as final, enabling better compiler devirtualization/inlining in the SDK’s HTTP upload and offline-storage code paths.
Changes:
- Mark concrete HTTP client implementations (
HttpClient_*) asfinal. - Mark concrete offline storage implementations/handler (
OfflineStorage_*,MemoryStorage,OfflineStorageHandler) asfinal.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| lib/offline/OfflineStorageHandler.hpp | Marks OfflineStorageHandler as final to enable devirtualization through known concrete type usage. |
| lib/offline/OfflineStorage_Room.hpp | Marks OfflineStorage_Room as final to allow devirtualization in Android Room-backed storage path. |
| lib/offline/MemoryStorage.hpp | Marks MemoryStorage as final to enable devirtualization for in-memory storage operations. |
| lib/http/HttpClient_WinRt.hpp | Marks HttpClient_WinRt as final to improve call-site optimization on WinRT HTTP path. |
| lib/http/HttpClient_WinInet.hpp | Marks HttpClient_WinInet as final to improve call-site optimization on WinInet HTTP path. |
| lib/http/HttpClient_Curl.hpp | Marks HttpClient_Curl as final to improve call-site optimization on Curl HTTP path. |
| lib/http/HttpClient_CAPI.hpp | Marks HttpClient_CAPI as final to improve call-site optimization for C API-backed HTTP client. |
| lib/http/HttpClient_Apple.hpp | Marks HttpClient_Apple as final to improve call-site optimization on Apple HTTP path. |
| lib/http/HttpClient_Android.hpp | Marks HttpClient_Android as final to improve call-site optimization on Android HTTP path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Companion to the `final` devirtualization in this PR: give internal linkage to translation-unit-local symbols so the compiler can inline / drop them and keep them out of the (static-archive and shared-object) symbol table. This helps the static-lib consumption path that -fvisibility=hidden does not fully cover, since hidden visibility only trims the dynamic export table while these symbols keep external linkage across TUs. lib/api/capi.cpp: mark the 10 file-local C-API dispatch helpers static (remove_client, mat_open_core, mat_open, mat_open_with_params, mat_log, mat_close, mat_pause, mat_resume, mat_upload, mat_flushAndTeardown). Verified each is called only from the single exported entry point evt_api_call_default (and each other) within capi.cpp, and appears in no header and no other translation unit. capi_get_client stays external (it is MAT::capi_get_client, declared in a header and used by the CAPI HTTP client). Consistent with the file's existing static mtx/clients. lib/shared/dllmain.cpp: mark thread_count static -- a file-scope mutable global mutated only inside DllMain in this TU. get_platform_uuid (sysinfo_sources.cpp) was deliberately NOT touched: it has no caller in-tree, so marking it static would trip -Wunused-function under -Werror. Verified: NDK clang aarch64 -fsyntax-only on capi.cpp is clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bmehta001
changed the title
Comment on lines
233
to
+236
| /** | ||
| * Marashal C struct to C++ API | ||
| */ | ||
| evt_status_t mat_log(evt_context_t *ctx) | ||
| static evt_status_t mat_log(evt_context_t *ctx) |
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.
Devirtualization: mark concrete leaf impl classes
finalMarks the leaf concrete implementations of the hot
IHttpClient/IOfflineStorageinterfaces
final, so the compiler can devirtualize (and frequently inline)virtual calls made through a known-final type ? smaller, faster code on every
HTTP upload and offline-storage operation.
Marked final (9):
HttpClient_Curl,HttpClient_WinInet,HttpClient_WinRt,HttpClient_Apple,HttpClient_Android,HttpClient_CAPIOfflineStorage_Room,MemoryStorage,OfflineStorageHandlerVerification: each was confirmed to have no subclass anywhere in the tree (
lib+tests+wrappers).Deliberately NOT marked:
OfflineStorage_SQLite?tests/unittests/OfflineStorageTests_SQLite.cppsubclasses it (OfflineStorage_SQLiteNoAutoCommit).TelemetrySystemBase? it's the base ofTelemetrySystem/AITelemetrySystem.Validation: NDK
aarch64 -fsyntax-onlyclean onOfflineStorage_Room,OfflineStorageHandler,MemoryStorage. The platform-specific HTTP impls (WinInet/WinRt/Apple) rely on the no-subclass verification + per-platform CI.Note on
static/anonymous-namespaceThe companion
static/anon-namespace cleanup for file-local helpers is a planned follow-up. Its primary benefit (keeping internals out of the exported symbol set) is largely already delivered by the-fvisibility=hiddenchange in the vcpkg PR (#1475), so it's lower priority;staticadds the incremental win of stronger inlining/DCE and no symbol entry at all, which can be a focused follow-up.Internal linkage (
static) — added in d0c0afbCompanion micro-optimization in the same spirit: give internal linkage to translation-unit-local symbols so the compiler can inline/drop them and keep them out of the symbol table. This specifically helps the static-library consumption path (e.g. vcpkg), which
-fvisibility=hiddendoes not fully cover — hidden visibility only trims a shared object's dynamic export table, whereas these symbols otherwise keep external linkage across TUs.lib/api/capi.cpp— marked the 10 file-local C-API dispatch helpersstatic(remove_client,mat_open_core,mat_open,mat_open_with_params,mat_log,mat_close,mat_pause,mat_resume,mat_upload,mat_flushAndTeardown). Each is called only from the single exported entry pointevt_api_call_default(and from one another) withincapi.cpp, and appears in no header and no other TU.MAT::capi_get_clientstays external (header-declared, used by the CAPI HTTP client).lib/shared/dllmain.cpp— markedthread_countstatic(file-scope global mutated only insideDllMain).Deliberately not touched:
get_platform_uuid(sysinfo_sources.cpp) has no in-tree caller, sostaticwould trip-Wunused-functionunder-Werror.Verified: NDK clang
aarch64 -fsyntax-onlyoncapi.cppis clean; whole-tree cross-reference confirmed none of the internalized symbols are referenced from any header or other translation unit.