EC: skip unchanged files in INC_UPDATE for big-library performance - #727
Conversation
|
Update — two follow-up commits address a protocol-invariant regression and extend the optimization to amuleweb. The bug (caught by @Stoatwblr in #713): amulegui's INC_UPDATE handler infers deletions from absence — any file ID missing from the response is removed from the local view. Skipping unchanged files entirely meant most of a 91k-file shareset disappeared every cycle, then the 5-min backstop re-added them, then they vanished again — wedging amulegui's list widget at 100% CPU on The fix — two new commits that keep the optimization in both paths:
Negotiation matrix:
Also drops the 5-minute paranoia backstop. With explicit removal markers and alive markers covering both protocol paths, the backstop's only remaining job was masking missed Wire-format additionsTwo new EC tag IDs in Old daemons / clients ignore unknown tags, so both halves of the negotiation are forward-compatible. Test plan update
Still draft pending real-workload soak. |
|
One more commit — Background: Hooks added (all conditional on actual value change):
Refreshed |
|
There is a conflict in KnownFile.cpp. |
Adds the data + plumbing for skipping unchanged files in INC_UPDATE responses (amule-project#713). No callers yet, no behavior change — pure infrastructure to make the next two commits reviewable on their own. Each CKnownFile carries an atomic uint64 generation counter. A process- wide atomic global counter ensures every Mark…() yields a strictly- ascending value across all files and all threads (CFileStatistic counters tick from upload-disk-IO + hashing threads, not just main). `MarkECChanged()` uses a single fetch_add(1) + store rather than a load / increment / store so concurrent mutations from different threads can't collide on the same gen. Memory cost: 8 bytes per CKnownFile (~700 KB for a 91 k-file shareset). One additional atomic uint64 at file scope. Refs amule-project#713.
Wires `MarkECChanged()` into each storage primitive that writes a field
exported via `CEC_SharedFile_Tag` / `CEC_PartFile_Tag`. Still no behavior
change on the EC dispatch side — the next commit consumes the flag.
Sites covered:
* `CFileStatistic::AddRequest / AddAccepted / AddTransferred`
(EC_TAG_KNOWNFILE_REQ_COUNT, _ACCEPT_COUNT, _XFERRED and their _ALL
counterparts). `AddTransferred` is the hottest of the three —
every uploaded chunk flips the flag. The Mark op is a single
atomic fetch_add+store so the cost is small even on multi-slot
upload nodes.
* `CKnownFile::AddUploadingClient / RemoveUploadingClient`
(EC_TAG_KNOWNFILE_ON_QUEUE = m_ClientUploadList.size()).
* `CKnownFile::SetFileName` (EC_TAG_PARTFILE_NAME and ed2k:// link).
* `CKnownFile::SetFilePath` (EC_TAG_KNOWNFILE_FILENAME on non-partfiles).
* `CKnownFile::SetUpPriority / SetAutoUpPriority` (EC_TAG_KNOWNFILE_PRIO,
which folds the auto flag in as +10).
* `CKnownFile::SetFileCommentRating` (EC_TAG_KNOWNFILE_COMMENT / _RATING).
* `CKnownFile::UpdatePartsInfo` (EC_TAG_KNOWNFILE_COMPLETE_SOURCES{,LOW,HIGH}).
* `m_pAICHHashSet->SetMasterHash` call sites in KnownFile.cpp /
PartFile.cpp and `SetAICHHashset` swap in amule.cpp's hash-task
completion (EC_TAG_KNOWNFILE_AICH_MASTERHASH).
* `CPartFile::Process` — coarse hook at the per-second per-partfile
tick. Partfiles have ~20 EC-exported fields that change
independently on active downloads; the per-second tick is the
right granularity and avoids hooking each of them.
A 60 s periodic backstop in commit (3/3) catches anything missed here.
Refs amule-project#713.
Consumes the per-file `m_ecGen` from commit (1/3) and the `MarkECChanged`
hooks from commit (2/3): `Get_EC_Response_GetUpdate` now skips files
whose generation is at or below the highest gen this connection has
already reflected in its `CObjTagMap`. For a 91 k-file shareset with a
few hundred actively-transferring files at any moment, the per-cycle
iteration goes from O(N) to O(files-that-actually-changed) — typically
a 99 %+ reduction.
Mechanism per connection:
* snapshot = CKnownFile::s_globalEcGen at request start
* threshold = m_lastEcGenSeen carried from the previous response
* iterate only files where file.m_ecGen > threshold
* m_lastEcGenSeen := snapshot after iteration
* files whose m_ecGen changes mid-iteration land above the snapshot
and get picked up on the next request
Backstop: every kFullEcSyncIntervalSeconds (60 s) every connection
runs an unconditional full sweep regardless of gen. Worst-case
staleness if a `MarkECChanged()` hook was missed somewhere is bounded
to that interval rather than indefinite. The interval is short enough
to keep amulegui's UI from drifting noticeably and long enough that
the 99 % skip win still dominates the per-cycle workload.
Wire format unchanged; INC_UPDATE responses now contain only the
files that changed since the last response, which is exactly what the
delta protocol was designed to convey but the implementation used to
emit empty tag bodies for unchanged files anyway. Backwards-compatible
with any client version (the client side already correctly handles
missing-file-tag = "no change").
Refs amule-project#713.
…t-in
Commit 3 broke amulegui on big libraries because amulegui infers
deletions from absence: any file ID missing from an INC_UPDATE response
is removed from the local view. Skipping unchanged files entirely meant
most of the shareset disappeared every cycle (then the 5-min backstop
re-added them), wedging the GUI's list-widget at 100% CPU in
std::unordered_map churn.
Two-pronged fix preserving the optimization in both compat and opt-in
paths:
* Backward compat (old amulegui / amuleweb / third-party client):
unchanged files get a 5-byte "alive marker" tag — EC_TAG_KNOWNFILE /
EC_TAG_PARTFILE with the ECID and no children. The legacy client
already gates updates on `if (tag->HasChildTags())`, so empty tags
count as "present" without triggering the per-item update. Keeps the
expensive CEC_SharedFile_Tag construction + encoder pass off the
hot path (the actual perf win); only pays a tiny per-file wire cost.
* Partial-update protocol opt-in:
- New EC_TAG_CAN_PARTIAL_UPDATE capability advertised by the client
at auth, mirrored back by the server in AUTH_OK if it understands
the protocol.
- When negotiated, server skips unchanged files entirely and emits
explicit EC_TAG_FILE_REMOVED markers (top-level, value = ECID)
for files that disappeared since the previous cycle.
CECServerSocket tracks the previous cycle's snapshot in
m_lastSentFileIds.
- amulegui clients in this mode skip the bulk "missing == deleted"
loop and process explicit removals only.
Also drops the 5-minute paranoia backstop. With explicit removal
markers and alive markers covering both protocol paths, the backstop's
only remaining job was hiding missed MarkECChanged() hooks — making
those bugs harder to find rather than easier.
Negotiation matrix:
new server + new client → partial-update (full win, ~0 work
per-cycle for static files)
new server + old client → alive markers (almost full win, cheap
per-file wire cost)
old server + new client → server ignores capability, doesn't mirror
it back; client falls back to bulk-deletion path
old server + old client → unchanged
amuleweb drives its INC_UPDATE polling through `EC_OP_GET_SHARED_FILES` + `EC_OP_GET_DLOAD_QUEUE` with `EC_DETAIL_UPDATE` rather than the `EC_OP_GET_UPDATE` path amulegui uses. Commit (4/5) wired the partial- update protocol only into `Get_EC_Response_GetUpdate`, so amuleweb kept iterating all files every cycle even on huge libraries. Extend the same skip-unchanged + `EC_TAG_FILE_REMOVED` logic into `Get_EC_Response_GetSharedFiles` and `Get_EC_Response_GetDownloadQueue`, guarded on the same `m_partialUpdateActive` flag and only kicking in for `EC_DETAIL_UPDATE` requests (so amulecmd's `show shared` and other `EC_DETAIL_FULL` callers are untouched). Per-path state because the three handlers fire on independent cadences: * `m_lastEcGenSeen` + `m_lastSentFileIds` — amulegui's `EC_OP_GET_UPDATE` * `m_lastEcGenSeenShared` + `m_lastSentSharedFileIds` — amuleweb's `EC_OP_GET_SHARED_FILES` * `m_lastEcGenSeenPart` + `m_lastSentPartFileIds` — amuleweb's `EC_OP_GET_DLOAD_QUEUE` amuleweb's `UpdatableItemsContainer::ProcessUpdate` learns the same two-path behaviour: when the negotiated capability is active, drain `EC_TAG_FILE_REMOVED` markers into a buffered removal set and run a single sweep over the live list only when removals exist; otherwise the legacy "missing == deleted" loop stays correct (server still emits diff-empty alive-marker tags via the existing encoder path). `CaMuleExternalConnector::IsServerPartialUpdateActive()` exposes `CRemoteConnect::ServerSupportsPartialUpdate()` to the container. Compat matrix unchanged from (4/5): the negotiation is symmetric, so old amuleweb + new amuled and new amuleweb + old amuled both fall back to the legacy path automatically.
…ction setters
Reverse audit of every EC_DETAIL_UPDATE field surfaced nine mutation
sites that change exported state without marking the partfile changed.
For an active partfile the per-tick `MarkECChanged()` at the top of
`CPartFile::Process()` covered them implicitly, but `DownloadQueue::
Process()` gates `Process()` to `PS_READY`/`PS_EMPTY` only — paused,
stopped, hashing, and completed partfiles never auto-mark, so a user
toggling pause/resume/priority/category from amulegui or amuleweb
would stay invisible until something else on the file changed.
The 5-minute paranoia backstop (removed in 4/5) used to paper over
this; explicit hooks are the correct fix.
Sites covered:
* `CPartFile::StopFile` — EC_TAG_PARTFILE_STOPPED / _STATUS
* `CPartFile::PauseFile` — EC_TAG_PARTFILE_STOPPED / _STATUS
* `CPartFile::ResumeFile` — EC_TAG_PARTFILE_STOPPED / _STATUS
* `CPartFile::SetDownPriority` — EC_TAG_PARTFILE_PRIO
* `CPartFile::SetAutoDownPriority` — EC_TAG_PARTFILE_PRIO (auto flag
folded in via +10 offset)
* `CPartFile::SetA4AFAuto` — EC_TAG_PARTFILE_A4AFAUTO
* `CPartFile::SetCategory` — EC_TAG_PARTFILE_CAT
* `CPartFile::SetHashingProgress` — EC_TAG_PARTFILE_HASHED_PART_COUNT
(runs during hashing, outside
Process()'s gate)
* `CPartFile::UpdateFileRatingCommentAvail` — EC_TAG_PARTFILE_COMMENTS
(fires when sources deliver
ratings/comments via OP_MESSAGE)
All hooks are conditional on actual value change to keep the
generation counter from incrementing on no-op writes.
Solved |
dfac5b0 to
035304c
Compare
…nownFile (amule-project#748) CKnownFilesRem::DeleteItem was destroying a CKnownFile without sweeping the client list to null out CUpDownClient::m_uploadingfile / m_reqfile on clients that still pointed at it. Sequence: 1. EC_TAG_FILE_REMOVED arrives for file X → CKnownFilesRem::DeleteItem(X) → X is destroyed. 2. Any CUpDownClient whose m_uploadingfile == X now holds a dangling pointer. 3. Some later INC_UPDATE batch removes that client → CUpDownClientListRem::DeleteItem(client) at line 1504 null-checks m_uploadingfile (non-null, passes the guard), line 1505 calls m_uploadingfile->RemoveUploadingClient(client) → operates on the freed CKnownFile, std::set::erase walks freed m_ClientUploadList nodes via CClientRef::operator< → SEGV. Fix is a single-line callout from CKnownFilesRem::DeleteItem to a new CUpDownClientListRem::DropReferencesTo(file) helper, which has the friend access to CUpDownClient's private fields that CKnownFilesRem lacks. O(N_clients) per file removal, bounded because EC_TAG_FILE_REMOVED markers are rare (only fire when amuled actually drops a known file, not on every INC_UPDATE). The bug existed pre-amule-project#727 in principle but the legacy "anything missing from the response is deleted" sweep tended to clean the dangling-pointer clients out of the list before the next time their DeleteItem ran. With explicit EC_TAG_FILE_REMOVED markers (opt-in via EC_TAG_CAN_PARTIAL_UPDATE), file deletions are rare and clients can hold the stale pointer arbitrarily long — heavy sharesets like the one reporting hit the timing window readily. Refs amule-project#748.
Summary
Get_EC_Response_GetUpdatecurrently iterates every file in the encoder map on every INC_UPDATE request, constructs a freshCEC_SharedFile_Tag/CEC_PartFile_Tagfor each one, and lets the per-connectionCObjTagMapdiff inside the constructor decide which fields to emit. On a 91 k-file shareset that's 91 k constructor invocations per cycle, with most of the work thrown away because the file's exported fields didn't change. Profiling (#713) showed this is the dominant remaining EC dispatch cost after #725.This PR adds a per-file change-generation counter that flips on every field mutation, and skips files whose generation hasn't advanced since the last response for that connection. For typical seedbox workloads where only a few hundred files of tens of thousands are actively transferring at any moment, per-cycle iteration drops from O(N) to O(files-actually-changed) — typically a 99 %+ reduction.
Scope: INC_UPDATE consumers only — amulegui and amuleweb. amulecmd is unaffected because amulecmd always issues FULL
GET_SHARED_FILES, not INC_UPDATE. The amulecmd FULL-response path is a separate optimisation target if needed.Three commits for reviewability
m_ecGen, process-wide atomics_globalEcGen,MarkECChanged()method. No callers, no behavior change.MarkECChanged()at every storage primitive that writes a field exported byCEC_SharedFile_Tag/CEC_PartFile_Tag:CFileStatistic::AddRequest / AddAccepted / AddTransferredCKnownFile::AddUploadingClient / RemoveUploadingClientCKnownFile::SetFileName / SetFilePath / SetUpPriority / SetAutoUpPriorityCKnownFile::SetFileCommentRatingCKnownFile::UpdatePartsInfoCKnownFile::Initso a freshly-constructed file is always above any existing thresholdCPartFile::Process(coarse — partfile has ~20 fields that change every tick during active download; per-second granularity matches reality)Get_EC_Response_GetUpdatesnapshotss_globalEcGen, skips files whosem_ecGen≤ the threshold from the last response, updates the threshold afterwards. Every 5 minutes a forced full sweep runs as a safety net for any missed hook — worst-case staleness if a hook was missed somewhere is bounded to that window rather than indefinite. 5 min is short enough to be invisible in the GUI and rare enough that the overhead is negligible (one full sweep per ~300 INC_UPDATE cycles at amulegui's ~1 Hz rate).Backwards compatibility
Wire format unchanged. INC_UPDATE responses now contain only the files that changed since the last response, which is exactly what the delta protocol was designed to convey but the implementation used to emit empty per-file tag bodies for unchanged files anyway. Backwards-compatible with any amulegui / amuleweb / amulecmd version, old or new — the client side already correctly handles "no file tag = no change for this file".
Memory cost
CKnownFile(atomic uint64) — ~700 KB on a 91 k-file shareset.CECServerSocketfor the per-connection tracking — negligible.Test plan
amuled,amulecmd,amulegui— clean compile, no crash on basic startup / shutdown sanity.Get_EC_Response_GetUpdateshould drop from ~59 % ofProcessRequest2(per the with-children profile in observation: amuled/amulgui interaction @ build 2.3.3-474-g56a369e95 (bottlenecks) #713) to a small fractionperf record -p amuled -g -F 99 -- sleep 30against a real workload to confirm the deltaRefs #713.