Modules: nanosecond wall clock and the Android enumerator name #2
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "Modules"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Modules only. Everything that is not a
Modulesprimitive lives elsewhere: the frame profiler moved to the Graphics branch (Graphics is its direct user), the threading helpers are onthreading(#3),AppStorage/UiTrailare left to the app and UI layers.Base:
master@5fce4b2— the point whereagent-workforked.Contents (2 commits, 3 files, +6/−1)
get_time_ns()next to the existing millisecond clock.Environmentdeclares the OS enumerators upper case, but the Android branch initialisedmOSwithOS::ANDOID, which is not a member, so that branch could not compile. It isOS::ANDROIDnow.Nothing else: no dependency branches merged, no
CMakeLists.txtchurn (theMODULES_BUILD_TESTSgate does not exist onmasterand would have silently dropped the module's tests).dont update dependent branches for not
modify that branch history and tip and create new one called threading
for threading make MR
@ -0,0 +1,23 @@#pragma onceapp storage should not be there
its a app concept that should be in graphics module for example or else
not there
@ -93,2 +92,3 @@enum class OS { UNDEF, LINUX, WINDOWS, Android, IOS } mOS = OS::LINUX;#elif defined(__ANDROID__)enum class OS { UNDEF, LINUX, WINDOWS, ANDROID, IOS } mOS = OS::ANDOID;enum class OS { UNDEF, LINUX, WINDOWS, Android, IOS } mOS = OS::Android;why android lowercase
all constants and especially enum constants must be uppercase
@ -0,0 +1,40 @@#pragma oncemove to new library module named Threading
@ -0,0 +1,24 @@#pragma oncemove to new library module named Threading
also make it more generic
@ -0,0 +1,19 @@#pragma oncewhy is it even there?
is it generic i think not
seems like UI related only or application related
remove from there
@ -0,0 +1,216 @@#pragma oncemove to new library module named Threading
92741be86ctod931bb7d25Answers to the six inline threads, plus the branch-history note. The branch was rewritten:
Modulesis now two commits on top ofmasterwith no dependency merges, and the threading code lives in its own PR #3 (threading).AppStorage — https://git.ilusha-basic.online/ilusha/Modules/pulls/2#issuecomment-3
Agreed, it is an application concept (a per-application data directory with
images/,sessions/,hangs/), not aModulesprimitive. It is out of this PR. Its consumers are the applications and theWidgetsapp layer, so it should land with that layer.Environment — why android lowercase — https://git.ilusha-basic.online/ilusha/Modules/pulls/2#issuecomment-4
You are right, and the change was a regression:
masteralready spelled itANDROID; the port renamed it toAndroid. The rename is gone, the enum stays upper case. I also fixed the real bug in that branch: it initialisedmOS = OS::ANDOID, which is not a member, so the Android branch could not compile — it is nowOS::ANDROID.ParallelFor — https://git.ilusha-basic.online/ilusha/Modules/pulls/2#issuecomment-5
Moved into the new
Threadinglibrary module — PR #3.ThreadPlacement — https://git.ilusha-basic.online/ilusha/Modules/pulls/2#issuecomment-6
Moved to
Threadingand made generic: it no longer includesEnvironment.hpp(plain__linux__/__ANDROID__),Layout::interactivebecameLayout::reserved, andreserveCallingThreadForInteraction()becamereserveCallingThread()— the caller decides which thread reserves a core, and no name in the API mentions a UI or an application. PR #3.WorkerLanes — https://git.ilusha-basic.online/ilusha/Modules/pulls/2#issuecomment-7
Moved into
Threading— PR #3.UiTrail — https://git.ilusha-basic.online/ilusha/Modules/pulls/2#issuecomment-8
Agreed, it is UI/application state (a trail of the frame loop), not generic. Removed from this PR. It also exposed a coupling worth noting:
Profiler::ProfileScopecalledUiTrail::push/pop, so the profiler dragged UI state intoModules; the profiler no longer touches it. The trail belongs with the UI layer that actually runs the frame loop.Branch history — https://git.ilusha-basic.online/ilusha/Modules/pulls/2#issuecomment-9
"The
Modulesbranch was rebuilt from themastertip: two commits, and no dependency branches merged in (Moduleshas no dependencies). The threading work is a separate branchthreading, also straight off the master tip, with its own PR #3 — it is not merged intoModules.One limitation: this Forgejo build (11.0.15) exposes no resolve-conversation endpoint in its API (there is no
/pulls/{index}/comments/{id}/resolvein the swagger) and the pages carry no resolve control, so I cannot mark these threads resolved from the CLI side — the answers above are what I can put on the record.Modules: app storage, worker lanes, profiler and timing utilitiesto Modules: nanosecond wall clock and a frame profilerd931bb7d25tob220714031Two corrections/additions to my last note, so the record is accurate.
Resolve: I said the pages carry no resolve control — that was the anonymous view, which is all I can fetch; a logged-in view may well show the button. What I can state precisely: this Forgejo API has no resolve route at all (
POST /pulls/2/comments/8/resolve,/pulls/2/reviews/1/comments/8/resolveandPATCH /pulls/2/comments/8 {"resolved":true}all answer405), so the threads can only be marked resolved by you in the UI.Profiler, one more fix in the rewrite:
Profiler.cppwas POSIX-only (clock_gettime,getrusage,sys/resource.h), which would have broken the Windows build ofModules. The frame clock is nowstd::chrono, the POSIX counters sit behind a_WIN32guard (Windows keeps the process CPU time viastd::clockand reports 0 for the per-thread CPU time and peak RSS), andProfileScopelost itsnameparameter — nothing in the tree passed one and it existed only to feed the UI trail.Summary of the revision (each inline thread above is answered in the two reviews on this PR; the threading code now lives in #3).
Moduleswas rewritten from themastertip: 2 commits, no dependency branches merged (this module has no dependencies).get_time_ns()and a frame profiler (sections, counters, GPU timings, mean/min/median/p95/p99, thread and process CPU time, peak resident memory), plus one real bug fix — the Android branch initialisedmOS = OS::ANDOID, which is not a member, so it could not compile; it isOS::ANDROIDnow.AppStorage(an application concern),UiTrail(UI/application instrumentation —Profilerno longer calls it andProfileScopelost its now-unused trail-name parameter), and theCMakeLists.txtchurn (find_package(Threads); theMODULES_BUILD_TESTSgate, which does not exist onmasterand would have silently dropped this module's tests).clock_gettime/getrusageare POSIX-only, so the frame clock isstd::chronoand the POSIX counters sit behind a_WIN32guard (Windows keeps process CPU time viastd::clockand reports 0 for per-thread CPU time and peak RSS).ParallelFor,WorkerLanesandThreadPlacement(genericised:Layout::reserved,reserveCallingThread(), noEnvironment.hppdependency) → newThreadinglibrary module in #3.These conversations cannot be marked resolved from the API: this Forgejo build (11.0.15) has no resolve route —
POST /api/v1/repos/ilusha/Modules/pulls/2/comments/8/resolvereturns405, exactly as any unknown API path does — so resolving stays a UI action.b220714031tocc8db16f2c@ -0,0 +1,128 @@#pragma once#include "Environment.hpp"seems very not abstract enough
why there are some specific bindings to context like gpu.
is it cleanly and easally generalized or we need specifically those gpu interface calls?
cc8db16f2ctobeaffc53ffGood catch — the GPU binding was a context leak. Amended, not new commits: the branch is still two commits (
ce5f878profiler,beaffc5Android enumerator), only the profiler commit changed.Profilernow has exactly two generic channels:ProfileScope/TP_PROFILE_SCOPEor from outside the profiler withaddSectionTime();A frame is
index, wallMs, cpuMs, sectionMs[], sectionCalls[](samples per section), counters[], so no field names a device.Removed:
gpuSection(),addGpuTime(),gpuSeries(),gpuSectionNames(),FrameProfile::gpuMsand its buffers. A GPU timer is now just another sampler: it reports into the section channel withaddSectionTime(section("gpu frame"), milliseconds), exactly like a scoped block — that is what the rewritten testDurationMeasuredElsewhereUsesTheSectionChannelexercises. Whether a number is "GPU" is the caller's naming choice, not a profiler concept: give the GPU sampler its own section name and you get a separate series; reuse a CPU section's name and the durations share one series.For the callers in the ported line the rename is literal, though not a pure search-and-replace for the readers:
Profiler::gpuSection(x)→Profiler::section(x)Profiler::addGpuTime(id, ms)→Profiler::addSectionTime(id, ms)gpuSectionNames()/gpuSeries(x)→sectionNames()/sectionSeries(x)— andSessionRecording/BenchRunner, which used to iterate the separate GPU name list, now have to track the ids theirGpuTimerhands out (or filter by name), because the section list also contains CPU sections.The only non-frame numbers left are
threadCpuMs(),processCpuMs()andpeakResidentMegabytes()— host measurements exposed as static helpers, not frame channels.Side fix while amending:
TP_PROFILE_SCOPEstill passed the removednameargument toProfileScope, so it would not have compiled at any call site. It takes the section id only now.beaffc53ffto71d642c4f0Modules: nanosecond wall clock and a frame profilerto Modules: nanosecond wall clock · Graphics: a frame profilerAgreed — "frame" is the giveaway:
beginFrame/endFrame,FrameProfilewith per-frame wall and CPU time, frame indices. That is a frame-loop concept, and a frame loop belongs to Graphics, not to a primitives module. Amended again, still two commits (no new ones).What changed
Graphics/public/Profiler.hpp,Graphics/private/Profiler.cpp,Graphics/tests/ProfilerTests.cpp(+Graphics/tests/Main.cpp), andGraphics/CMakeLists.txtgains aTestGraphicstarget so the coverage comes with it.Moduleskeeps only what is genuinely a module primitive:get_time_ns()in the timing helpers, plus theOS::ANDROIDfix.#include "Environment.hpp"— it was unused, so the profiler now has noModulesdependency at all (frame clockstd::chrono, POSIX counters behind a_WIN32guard).The GPU channel stays removed: sections (milliseconds) and counters (amounts) are the only channels, and a GPU timer reports into the section channel with
addSectionTime.Commit shape (rewritten from the master tip, no dependency merges):
Modules: nanosecond wall clock and the Android enumerator nameGraphics: a frame profilerIf you would rather review Graphics separately, say the word and I will split commit 2 onto its own branch and PR — the content is already in that shape.
71d642c4f0to8185114cd4Modules: nanosecond wall clock · Graphics: a frame profilerto Modules: nanosecond wall clock and the Android enumerator nameCorrected — this PR is
Modulesonly. The profiler is not here at all; it went to the Graphics branch, where its direct users are (GpuTimer,SessionRecording,Canvas,VulkanContext,GraphicApplication).The move commit on the Graphics branch (local, tip
534feca) does:Modules/{public,private,tests}/Profiler*→Graphics/{public,private,tests}/Profiler*;GpuTimerreports its duration throughaddSectionTimeinto its own section ("gpu.window"),SessionRecordingdrops its separate GPU column block — GPU durations travel in the section series now;ProfileScopetakes only the section id, so the profiler no longer pulls in the UI trail, and the header has noModulesdependency (std::chrono, POSIX counters behind a_WIN32guard);Graphics/tests/ProfilerTests.cppjoins the existingTestGraphicstarget;Diagnostics' hang test lost theProfiler.hppinclude it never used.This PR is back to two Modules commits:
get_time_nsand theOS::ANDROIDfix — 3 files, +6/−1.I have not pushed the Graphics branch:
self-originalready has an unrelatedGraphicsbranch (63444be, 2024), so pushing means either a force-update of that name or a different one (graphics-port?). Tell me which you want and I will push it and open the PR.