Switch attribution on and off without a restart - #53
Merged
Merged
Conversation
Per-mod tick attribution was config plus restart, which is the wrong shape for a profiler: the moment you want it is while the server is struggling, and restarting erases what you wanted to look at. /pulse attribution on and off drive the duty cycle on the running server and never write pulse.json, so a ten minute look does not become permanent by accident. /pulse attribution status says what is running and what it has measured. /pulse reload re-reads the file through the same loader startup uses, applies the Attribution block live, and names the keys whose values have drifted from what the server is running, because those still need a restart. All of it behind controlserver, so a panel console can drive it. The four families and the frame profiler priming are now registered whether or not Attribution.Enabled is set. That is what makes a later switch-on safe rather than merely likely to work: FrameProfilerUtil.End dereferences a root range that only Begin creates, and enabling the flag part-way through a tick on a profiler that has never completed one takes the server down. Priming before the tick loop guarantees the root exists, and PrevRootEntry being non-null is the proof the duty cycle checks. The cost is two profiled ticks at boot and four instruments nothing records into, and an instrument with no measurement is not a series. TickAttribution takes a new duty cycle at runtime by restarting the cycle, so switching off mid-burst drops the half-folded sample instead of publishing it and switching back on discards the stale tree the same way a normal burst start does.
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.
A hosting provider running Pulse asked whether the profiler can be switched without restarting the
server. It could not, and that is the wrong shape for a profiler: the moment you want per-mod
attribution is while the server is struggling, and a restart erases what you wanted to look at.
/pulsefixes it. Four subcommands, all behindcontrolserverso admins and a panel console canrun them:
/pulse attribution onstarts the duty cycle on whateverBurstTicksandIntervalSecondsarein force, with the first burst one interval later.
/pulse attribution offstops it and puts the engine's frame profiler back down on the next tick.A burst in progress is dropped rather than published half-measured.
/pulse attribution statusreports the cycle in use, the ticks profiled so far, and whether it isinside a burst right now.
/pulse reloadre-readspulse.jsonthrough the sameLoadModConfigandConfigUpgradepathstartup uses, applies the
Attributionblock live, and names the keys whose value in the file hasdrifted from what the server is running, since those still need a restart. A file that does not
parse changes nothing and the reply carries the parse error.
onandoffnever writepulse.json, and both replies say so. A ten minute look should not becomepermanent because somebody forgot to turn it off.
What made this safe
The four families and the frame profiler priming are now registered whether or not
Attribution.Enabledis set, and that is the load-bearing part rather than a tidy-up.FrameProfilerUtil.Enddereferences the root range that onlyBegincreates, andServerMaincalls
Endoutside the try/catch guarding the tick. Flipping the flag part-way through a tick on aprofiler that has never completed one is a NullReferenceException that takes the process down.
Priming at
RunGame, before the tick loop exists, guarantees the root is there, and the duty cyclealready refuses to touch the flag until
PrevRootEntryproves it. Arming a server that never asksfor attribution costs two profiled ticks at boot and four instruments nothing records into, and an
instrument with no measurement is not a series: an idle server serves the exposition it always did,
which the new scenario asserts before switching anything on.
TickAttributiontakes a new duty cycle at runtime by restarting the cycle rather than adjusting itin place. Switching off mid-burst therefore drops the half-folded sample instead of publishing it,
and switching back on discards the stale tree the profiler left behind exactly the way a normal
burst start does.
Tests
217 green, up from 195: 14 unit tests on the reply text and the restart-key comparison, 6 more on the
mutable duty cycle, and two Atlas scenarios. The first boots with attribution off, asserts nothing
attribution-shaped is on the wire, runs
/pulse attribution onthroughIWorldSession.ExecuteCommand(a console caller with every privilege, the same shapeServerMainbuilds for a console line), waits out a real burst, finds Pulse in its own numbers, then switches
off and checks both that
FrameProfiler.Enabledwent back down and that no further ticks accrue overthe next 300. The second rewrites
pulse.jsonunder the running server three times: a newBurstTicksthat reload applies, a changedPortthat it can only name, and a file that is not JSONat all, which leaves everything as it was.
tools/mutation-check.shgains three mutations and reports 44/44 killed. The reply string is aformat string with the parse error as a message parameter rather than interpolated, because the
engine runs a command reply through
string.Formaton its way to whoever asked and a JSON errorquoting a stray brace would blow that up.